binutils: build with LLVM 6.0.0

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

binutils: build with LLVM 6.0.0

Patrick Wildt-3
Hi,

LLVM 6.0.0 does now complain of code does computation on NULL pointers,
which apparently binutils makes use of.  I think we can teach binutils
to stop doing that.

Is my C foo correct?  Feedback?

Patrick

diff --git a/gnu/usr.bin/binutils-2.17/bfd/elflink.c b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
index a6d17dcb4d9..aa010f9d0b2 100644
--- a/gnu/usr.bin/binutils-2.17/bfd/elflink.c
+++ b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
@@ -4517,7 +4517,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
   len = strlen (name);
   copy = bfd_alloc (abfd, len);
   if (copy == NULL)
-    return (struct elf_link_hash_entry *) 0 - 1;
+    return (struct elf_link_hash_entry *)-1;
 
   first = p - name + 1;
   memcpy (copy, name, first);
@@ -4629,7 +4629,7 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
     }
 
   h = archive_symbol_lookup (abfd, info, symdef->name);
-  if (h == (struct elf_link_hash_entry *) 0 - 1)
+  if (h == (struct elf_link_hash_entry *)-1)
     goto error_return;
 
   if (h == NULL)
diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h b/gnu/usr.bin/binutils-2.17/include/obstack.h
index 88c2a264adc..8839c48e95f 100644
--- a/gnu/usr.bin/binutils-2.17/include/obstack.h
+++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
@@ -123,7 +123,7 @@ extern "C" {
 #endif
 
 #ifndef __INT_TO_PTR
-# define __INT_TO_PTR(P) ((P) + (char *) 0)
+# define __INT_TO_PTR(P) ((char *)(P))
 #endif
 
 /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

William Ahern-2
On Thu, Mar 15, 2018 at 05:23:24PM +0100, Patrick Wildt wrote:
> Hi,
>
> LLVM 6.0.0 does now complain of code does computation on NULL pointers,
> which apparently binutils makes use of.  I think we can teach binutils
> to stop doing that.
>
> Is my C foo correct?  Feedback?

Both (type *)0 - 1 and (type *)-1 rely on undefined behavior, but
_different_ undefined behavior. Even presuming the former was reliable, is
the latter?

FWIW, the two expressions also evaluate to different values,
0xfffffffffffffffc vs 0xffffffffffffffff. That's not a problem by itself but
suggests possible signedness (or whatever you call the analogous issue for
pointer arithmetic) pitfalls.

Maybe it's safer to use the address of abfd, which should be shared even if
archive_symbol_lookup is a function pointer to an external instantiation? I
originally thought to use a static global singleton, but the macros and
function pointers made me doubt the caller and callee are always from the
same object. Using abfd is simpler and I don't see where abfd might be
wrapped or proxied.

> Patrick
>
> diff --git a/gnu/usr.bin/binutils-2.17/bfd/elflink.c b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> index a6d17dcb4d9..aa010f9d0b2 100644
> --- a/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> +++ b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> @@ -4517,7 +4517,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
>    len = strlen (name);
>    copy = bfd_alloc (abfd, len);
>    if (copy == NULL)
> -    return (struct elf_link_hash_entry *) 0 - 1;
> +    return (struct elf_link_hash_entry *)-1;
>  
>    first = p - name + 1;
>    memcpy (copy, name, first);
> @@ -4629,7 +4629,7 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
>      }
>  
>    h = archive_symbol_lookup (abfd, info, symdef->name);
> -  if (h == (struct elf_link_hash_entry *) 0 - 1)
> +  if (h == (struct elf_link_hash_entry *)-1)
>      goto error_return;
>  
>    if (h == NULL)
> diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h b/gnu/usr.bin/binutils-2.17/include/obstack.h
> index 88c2a264adc..8839c48e95f 100644
> --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> @@ -123,7 +123,7 @@ extern "C" {
>  #endif
>  
>  #ifndef __INT_TO_PTR
> -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> +# define __INT_TO_PTR(P) ((char *)(P))
>  #endif
>  
>  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

Todd C. Miller-2
In reply to this post by Patrick Wildt-3
On Thu, 15 Mar 2018 17:23:24 +0100, Patrick Wildt wrote:

> diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h b/gnu/usr.bin/binuti
> ls-2.17/include/obstack.h
> index 88c2a264adc..8839c48e95f 100644
> --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> @@ -123,7 +123,7 @@ extern "C" {
>  #endif
>  
>  #ifndef __INT_TO_PTR
> -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> +# define __INT_TO_PTR(P) ((char *)(P))
>  #endif
>  
>  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is

Won't this cause a warning on 64-bit systems?  Maybe instead:

# define __INT_TO_PTR(P) ((char *)(intptr_t)(P))

Also, what about __PTR_TO_INT?  Perhaps:

# define __PTR_TO_INT(P) ((int)(intptr_t)(P))

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

Patrick Wildt-3
In reply to this post by William Ahern-2
On Thu, Mar 15, 2018 at 03:55:25PM -0700, William Ahern wrote:

> On Thu, Mar 15, 2018 at 05:23:24PM +0100, Patrick Wildt wrote:
> > Hi,
> >
> > LLVM 6.0.0 does now complain of code does computation on NULL pointers,
> > which apparently binutils makes use of.  I think we can teach binutils
> > to stop doing that.
> >
> > Is my C foo correct?  Feedback?
>
> Both (type *)0 - 1 and (type *)-1 rely on undefined behavior, but
> _different_ undefined behavior. Even presuming the former was reliable, is
> the latter?
>
> FWIW, the two expressions also evaluate to different values,
> 0xfffffffffffffffc vs 0xffffffffffffffff. That's not a problem by itself but
> suggests possible signedness (or whatever you call the analogous issue for
> pointer arithmetic) pitfalls.

You're right.  It does have a different value.  On the other hand, the
code is only used in three places.  One place checks for NULL, the other
two places are part of the diff.  And the comment above _bfd_elf_archive
_symbol_lookup actually says:

/* Return the linker hash table entry of a symbol that might be
   satisfied by an archive symbol.  Return -1 on error.  */

> Maybe it's safer to use the address of abfd, which should be shared even if
> archive_symbol_lookup is a function pointer to an external instantiation? I
> originally thought to use a static global singleton, but the macros and
> function pointers made me doubt the caller and callee are always from the
> same object. Using abfd is simpler and I don't see where abfd might be
> wrapped or proxied.

I don't understand what you're saying, I don't know the binutils code
well enough for that.

> > Patrick
> >
> > diff --git a/gnu/usr.bin/binutils-2.17/bfd/elflink.c b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > index a6d17dcb4d9..aa010f9d0b2 100644
> > --- a/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > +++ b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > @@ -4517,7 +4517,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
> >    len = strlen (name);
> >    copy = bfd_alloc (abfd, len);
> >    if (copy == NULL)
> > -    return (struct elf_link_hash_entry *) 0 - 1;
> > +    return (struct elf_link_hash_entry *)-1;
> >  
> >    first = p - name + 1;
> >    memcpy (copy, name, first);
> > @@ -4629,7 +4629,7 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
> >      }
> >  
> >    h = archive_symbol_lookup (abfd, info, symdef->name);
> > -  if (h == (struct elf_link_hash_entry *) 0 - 1)
> > +  if (h == (struct elf_link_hash_entry *)-1)
> >      goto error_return;
> >  
> >    if (h == NULL)
> > diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > index 88c2a264adc..8839c48e95f 100644
> > --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> > +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > @@ -123,7 +123,7 @@ extern "C" {
> >  #endif
> >  
> >  #ifndef __INT_TO_PTR
> > -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> > +# define __INT_TO_PTR(P) ((char *)(P))
> >  #endif
> >  
> >  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

Patrick Wildt-3
In reply to this post by Todd C. Miller-2
On Thu, Mar 15, 2018 at 09:31:13PM -0600, Todd C. Miller wrote:

> On Thu, 15 Mar 2018 17:23:24 +0100, Patrick Wildt wrote:
>
> > diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h b/gnu/usr.bin/binuti
> > ls-2.17/include/obstack.h
> > index 88c2a264adc..8839c48e95f 100644
> > --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> > +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > @@ -123,7 +123,7 @@ extern "C" {
> >  #endif
> >  
> >  #ifndef __INT_TO_PTR
> > -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> > +# define __INT_TO_PTR(P) ((char *)(P))
> >  #endif
> >  
> >  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is
>
> Won't this cause a warning on 64-bit systems?  Maybe instead:
>
> # define __INT_TO_PTR(P) ((char *)(intptr_t)(P))

Won't this sign extend the int?

> Also, what about __PTR_TO_INT?  Perhaps:
>
> # define __PTR_TO_INT(P) ((int)(intptr_t)(P))

Won't the (int) cut off the value on 64-bit systems?

I'm not sure that's what they are after at this point.  I think what
they are doing is converting a pointer to a "number" to do arithmetics
on.  So maybe it's

# define __INT_TO_PTR(P) ((char *)(uintptr_t)(P))
# define __PTR_TO_INT(P) ((uintptr_t)(P))

Patrick

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

Mark Kettenis
In reply to this post by Patrick Wildt-3
> Date: Sat, 31 Mar 2018 21:58:06 +0200
> From: Patrick Wildt <[hidden email]>
>
> On Thu, Mar 15, 2018 at 03:55:25PM -0700, William Ahern wrote:
> > On Thu, Mar 15, 2018 at 05:23:24PM +0100, Patrick Wildt wrote:
> > > Hi,
> > >
> > > LLVM 6.0.0 does now complain of code does computation on NULL pointers,
> > > which apparently binutils makes use of.  I think we can teach binutils
> > > to stop doing that.
> > >
> > > Is my C foo correct?  Feedback?
> >
> > Both (type *)0 - 1 and (type *)-1 rely on undefined behavior, but
> > _different_ undefined behavior. Even presuming the former was reliable, is
> > the latter?
> >
> > FWIW, the two expressions also evaluate to different values,
> > 0xfffffffffffffffc vs 0xffffffffffffffff. That's not a problem by itself but
> > suggests possible signedness (or whatever you call the analogous issue for
> > pointer arithmetic) pitfalls.
>
> You're right.  It does have a different value.  On the other hand, the
> code is only used in three places.  One place checks for NULL, the other
> two places are part of the diff.  And the comment above _bfd_elf_archive
> _symbol_lookup actually says:
>
> /* Return the linker hash table entry of a symbol that might be
>    satisfied by an archive symbol.  Return -1 on error.  */
>
> > Maybe it's safer to use the address of abfd, which should be shared even if
> > archive_symbol_lookup is a function pointer to an external instantiation? I
> > originally thought to use a static global singleton, but the macros and
> > function pointers made me doubt the caller and callee are always from the
> > same object. Using abfd is simpler and I don't see where abfd might be
> > wrapped or proxied.
>
> I don't understand what you're saying, I don't know the binutils code
> well enough for that.

FWIW, FreeBSD fixed it in a different way:

  https://reviews.freebsd.org/D12928

But upstream did it your way, at least for the elflink.c.  So the
elflink.c bit is ok kettenis@

I'm not sure about the obstack.h bit; why does it complain about
__INT_TO_PTR but not about __PTR_TO_INT?

Maybe we should just disable the warning in question for this crufty
code?  

> > > diff --git a/gnu/usr.bin/binutils-2.17/bfd/elflink.c b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > > index a6d17dcb4d9..aa010f9d0b2 100644
> > > --- a/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > > +++ b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > > @@ -4517,7 +4517,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
> > >    len = strlen (name);
> > >    copy = bfd_alloc (abfd, len);
> > >    if (copy == NULL)
> > > -    return (struct elf_link_hash_entry *) 0 - 1;
> > > +    return (struct elf_link_hash_entry *)-1;
> > >  
> > >    first = p - name + 1;
> > >    memcpy (copy, name, first);
> > > @@ -4629,7 +4629,7 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
> > >      }
> > >  
> > >    h = archive_symbol_lookup (abfd, info, symdef->name);
> > > -  if (h == (struct elf_link_hash_entry *) 0 - 1)
> > > +  if (h == (struct elf_link_hash_entry *)-1)
> > >      goto error_return;
> > >  
> > >    if (h == NULL)
> > > diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > > index 88c2a264adc..8839c48e95f 100644
> > > --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> > > +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > > @@ -123,7 +123,7 @@ extern "C" {
> > >  #endif
> > >  
> > >  #ifndef __INT_TO_PTR
> > > -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> > > +# define __INT_TO_PTR(P) ((char *)(P))
> > >  #endif
> > >  
> > >  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is
>
>

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

Patrick Wildt-3
On Sat, Mar 31, 2018 at 10:57:38PM +0200, Mark Kettenis wrote:
> I'm not sure about the obstack.h bit; why does it complain about
> __INT_TO_PTR but not about __PTR_TO_INT?

So, I think what the code in question is trying to do is convert a ptr
to a a different type while keeping the same "number" in memory.  It's
just a way so they can convert something to an integer to add/remove
values from.  And then it's converted back to a pointer type.

> Maybe we should just disable the warning in question for this crufty
> code?  

But this might be a lot easier actually.  Following diff works for me.
Does this still compile with LLVM 5.0.1?  I have no machine to test
at the moment.

Patrick

diff --git a/gnu/usr.bin/binutils-2.17/gas/configure b/gnu/usr.bin/binutils-2.17/gas/configure
index dca6497031c..39ce1f28621 100755
--- a/gnu/usr.bin/binutils-2.17/gas/configure
+++ b/gnu/usr.bin/binutils-2.17/gas/configure
@@ -4180,6 +4180,7 @@ using_cgen=no
 
 
 GCC_WARN_CFLAGS="-W -Wall -Wstrict-prototypes -Wmissing-prototypes"
+GCC_WARN_CFLAGS="$GCC_WARN_CFLAGS -Wno-null-pointer-arithmetic"
 
 # Check whether --enable-werror or --disable-werror was given.
 if test "${enable_werror+set}" = set; then
diff --git a/gnu/usr.bin/binutils-2.17/ld/configure b/gnu/usr.bin/binutils-2.17/ld/configure
index 0cd6f5c99d1..610b6a6caaa 100755
--- a/gnu/usr.bin/binutils-2.17/ld/configure
+++ b/gnu/usr.bin/binutils-2.17/ld/configure
@@ -4229,6 +4229,7 @@ fi;
 
 
 GCC_WARN_CFLAGS="-W -Wall -Wstrict-prototypes -Wmissing-prototypes"
+GCC_WARN_CFLAGS="$GCC_WARN_CFLAGS -Wno-null-pointer-arithmetic"
 
 # Check whether --enable-werror or --disable-werror was given.
 if test "${enable_werror+set}" = set; then

Reply | Threaded
Open this post in threaded view
|

Re: binutils: build with LLVM 6.0.0

Alexander Bluhm
On Thu, Apr 05, 2018 at 06:06:59PM +0200, Patrick Wildt wrote:
> Does this still compile with LLVM 5.0.1?

No.

cc -DHAVE_CONFIG_H -I. -I/usr/src/gnu/usr.bin/binutils-2.17/gas -I. -D_GNU_SOURCE -I. -I/usr/src/gnu/usr.bin/binutils-2.17/gas -I../bfd -I/usr/src/gnu/usr.bin/binutils-2.17/gas/config -I/usr/src/gnu/usr.bin/binutils-2.17/gas/../include -I/usr/src/gnu/usr.bin/binutils-2.17/gas/.. -I/usr/src/gnu/usr.bin/binutils-2.17/gas/../bfd -I/usr/src/gnu/usr.bin/binutils-2.17/gas/../intl -I../intl -DLOCALEDIR="\"/usr/share/locale\""    -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wno-null-pointer-arithmetic -Werror -O2 -pipe  -DPIE_DEFAULT=1  -c /usr/src/gnu/usr.bin/binutils-2.17/gas/app.c
error: unknown warning option '-Wno-null-pointer-arithmetic'; did you mean    
      '-Wno-null-arithmetic'? [-Werror,-Wunknown-warning-option]              
*** Error 1 in obj/gas (Makefile:2566 'app.o')                                

root@ot6:.../binutils-2.17# cc --version
OpenBSD clang version 5.0.1 (tags/RELEASE_501/final) (based on LLVM 5.0.1)    
Target: amd64-unknown-openbsd6.3
Thread model: posix
InstalledDir: /usr/bin

kern.version=OpenBSD 6.3-current (GENERIC.MP) #0: Thu Apr  5 17:02:35 CEST 2018
    [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP

bluhm