[PATCH] objcopy: fix 32-bit build

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

[PATCH] objcopy: fix 32-bit build

Jan Beulich-2
I do assume this isn't sufficient to address the non-64-bit-BFD case as
well, but it's not entirely clear what the right approach to deal with
that would be. I also wonder whether it was really meant to have 59 bits
set here, rather than 63.

binutils/
2018-01-10  Jan Beulich  <[hidden email]>

        * objcopy.c (merge_gnu_build_notes): Replace 64-bit constants by
        equivalent expressions.

--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asect
      For now though, since v1 and v2 was not intended to
      handle gaps, we chose an artificially large end
      address.  */
-  end = (bfd_vma) 0x7ffffffffffffffUL;
+  end = ~((bfd_vma)1 << 63);
   break;
   
  case 8:
@@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asect
  For now though, since v1 and v2 was not intended to
  handle gaps, we chose an artificially large end
  address.  */
-      end = (bfd_vma) 0x7ffffffffffffffUL;
+      end = ~((bfd_vma)1 << 63);
     }
   break;
 



Reply | Threaded
Open this post in threaded view
|

Ping: [PATCH] objcopy: fix 32-bit build

Jan Beulich-2
>>> On 10.01.18 at 10:32, <[hidden email]> wrote:
> I do assume this isn't sufficient to address the non-64-bit-BFD case as
> well, but it's not entirely clear what the right approach to deal with
> that would be. I also wonder whether it was really meant to have 59 bits
> set here, rather than 63.

Anyone? Or did I overlook some better fix which may have gone in
in the meantime?

Jan

> binutils/
> 2018-01-10  Jan Beulich  <[hidden email]>
>
> * objcopy.c (merge_gnu_build_notes): Replace 64-bit constants by
> equivalent expressions.
>
> --- a/binutils/objcopy.c
> +++ b/binutils/objcopy.c
> @@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asect
>       For now though, since v1 and v2 was not intended to
>       handle gaps, we chose an artificially large end
>       address.  */
> -  end = (bfd_vma) 0x7ffffffffffffffUL;
> +  end = ~((bfd_vma)1 << 63);
>    break;
>    
>   case 8:
> @@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asect
>   For now though, since v1 and v2 was not intended to
>   handle gaps, we chose an artificially large end
>   address.  */
> -      end = (bfd_vma) 0x7ffffffffffffffUL;
> +      end = ~((bfd_vma)1 << 63);
>      }
>    break;
>  



Reply | Threaded
Open this post in threaded view
|

Re: Ping: [PATCH] objcopy: fix 32-bit build

Alan Modra-3
On Wed, Jan 24, 2018 at 12:53:17AM -0700, Jan Beulich wrote:

> >>> On 10.01.18 at 10:32, <[hidden email]> wrote:
> > --- a/binutils/objcopy.c
> > +++ b/binutils/objcopy.c
> > @@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asect
> >       For now though, since v1 and v2 was not intended to
> >       handle gaps, we chose an artificially large end
> >       address.  */
> > -  end = (bfd_vma) 0x7ffffffffffffffUL;
> > +  end = ~((bfd_vma)1 << 63);
> >    break;
> >    
> >   case 8:
> > @@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asect
> >   For now though, since v1 and v2 was not intended to
> >   handle gaps, we chose an artificially large end
> >   address.  */
> > -      end = (bfd_vma) 0x7ffffffffffffffUL;
> > +      end = ~((bfd_vma)1 << 63);
> >      }
> >    break;

Why not use "(bfd_vma) -1"?  Won't this complain about shift count
exceeding width of type when bfd_vma is 32-bit?  (And if you don't get
a warning, there is at least a possibility of an all-ones bit pattern
when 32-bit while 64-bit will have the MSB zero.  A difference I don't
like.)

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: Ping: [PATCH] objcopy: fix 32-bit build

Jan Beulich-2
>>> On 29.01.18 at 00:04, <[hidden email]> wrote:
> On Wed, Jan 24, 2018 at 12:53:17AM -0700, Jan Beulich wrote:
>> >>> On 10.01.18 at 10:32, <[hidden email]> wrote:
>> > --- a/binutils/objcopy.c
>> > +++ b/binutils/objcopy.c
>> > @@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asect
>> >       For now though, since v1 and v2 was not intended to
>> >       handle gaps, we chose an artificially large end
>> >       address.  */
>> > -  end = (bfd_vma) 0x7ffffffffffffffUL;
>> > +  end = ~((bfd_vma)1 << 63);
>> >    break;
>> >    
>> >   case 8:
>> > @@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asect
>> >   For now though, since v1 and v2 was not intended to
>> >   handle gaps, we chose an artificially large end
>> >   address.  */
>> > -      end = (bfd_vma) 0x7ffffffffffffffUL;
>> > +      end = ~((bfd_vma)1 << 63);
>> >      }
>> >    break;
>
> Why not use "(bfd_vma) -1"?

Well - why was the original value not 0xffffffffffffffff? (I notice I've
actually submitted a stale patch - the proper value above ought to
be ~((bfd_vma)0x1f << 59) if we want to retain the current value,
as I had noticed only later, and then I apparently forgot to refresh
the patch. I did raise this point in the description though.)

>  Won't this complain about shift count
> exceeding width of type when bfd_vma is 32-bit?

Yes, I expect that. Hence me saying "I do assume this isn't sufficient
to address the non-64-bit-BFD case as well" in the description.

>  (And if you don't get
> a warning, there is at least a possibility of an all-ones bit pattern
> when 32-bit while 64-bit will have the MSB zero.  A difference I don't
> like.)

Ideally the original author would fix this, as I can second-guess what
the intention was with the value chosen. The change here is what I
minimally (continue to) need to be able to build and test on a 32-bit
host. Once I build 2.30 for a wider range of environments, I'm sure
I'll have to extend the fix/workaround. (I would really have hoped
for 2.30 to ship without such a build issue, or at the very least to
have a clear understanding of the original intentions here by now,
so I know how to "properly" work around this.)

Jan

Reply | Threaded
Open this post in threaded view
|

Re: Ping: [PATCH] objcopy: fix 32-bit build

Alan Modra-3
On Mon, Jan 29, 2018 at 02:43:40AM -0700, Jan Beulich wrote:

> >>> On 29.01.18 at 00:04, <[hidden email]> wrote:
> > On Wed, Jan 24, 2018 at 12:53:17AM -0700, Jan Beulich wrote:
> >> >>> On 10.01.18 at 10:32, <[hidden email]> wrote:
> >> > --- a/binutils/objcopy.c
> >> > +++ b/binutils/objcopy.c
> >> > @@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asect
> >> >       For now though, since v1 and v2 was not intended to
> >> >       handle gaps, we chose an artificially large end
> >> >       address.  */
> >> > -  end = (bfd_vma) 0x7ffffffffffffffUL;
> >> > +  end = ~((bfd_vma)1 << 63);
> >> >    break;
> >> >    
> >> >   case 8:
> >> > @@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asect
> >> >   For now though, since v1 and v2 was not intended to
> >> >   handle gaps, we chose an artificially large end
> >> >   address.  */
> >> > -      end = (bfd_vma) 0x7ffffffffffffffUL;
> >> > +      end = ~((bfd_vma)1 << 63);
> >> >      }
> >> >    break;
> >
> > Why not use "(bfd_vma) -1"?
>
> Well - why was the original value not 0xffffffffffffffff?

I suspect the intent was to use the largest positive bfd_signed_vma,
but Nick typoed the number of 'f' chars, and of course got it wrong
for 32-bit.  "(bfd_vma) -1 >> 1" would have been right if that was
his intention.  But the value used does not matter much.  Any value
hugely larger than what is expected for a .gnu.build.attributes should
work from what I can see of the code.

I'm going to commit the following.

        * objcopy.c (merge_gnu_build_notes): Use (bfd_vma) -1 as
        "artificially large" end address.

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 1e39f6d..8cdf27a 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
      For now though, since v1 and v2 was not intended to
      handle gaps, we chose an artificially large end
      address.  */
-  end = (bfd_vma) 0x7ffffffffffffffUL;
+  end = (bfd_vma) -1;
   break;
   
  case 8:
@@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
  For now though, since v1 and v2 was not intended to
  handle gaps, we chose an artificially large end
  address.  */
-      end = (bfd_vma) 0x7ffffffffffffffUL;
+      end = (bfd_vma) -1;
     }
   break;
 

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: Ping: [PATCH] objcopy: fix 32-bit build

Jan Beulich-2
>>> On 30.01.18 at 01:13, <[hidden email]> wrote:
> On Mon, Jan 29, 2018 at 02:43:40AM -0700, Jan Beulich wrote:
>> >>> On 29.01.18 at 00:04, <[hidden email]> wrote:
>> > On Wed, Jan 24, 2018 at 12:53:17AM -0700, Jan Beulich wrote:
>> >> >>> On 10.01.18 at 10:32, <[hidden email]> wrote:
>> >> > --- a/binutils/objcopy.c
>> >> > +++ b/binutils/objcopy.c
>> >> > @@ -2064,7 +2064,7 @@ merge_gnu_build_notes (bfd * abfd, asect
>> >> >       For now though, since v1 and v2 was not intended to
>> >> >       handle gaps, we chose an artificially large end
>> >> >       address.  */
>> >> > -  end = (bfd_vma) 0x7ffffffffffffffUL;
>> >> > +  end = ~((bfd_vma)1 << 63);
>> >> >    break;
>> >> >    
>> >> >   case 8:
>> >> > @@ -2083,7 +2083,7 @@ merge_gnu_build_notes (bfd * abfd, asect
>> >> >   For now though, since v1 and v2 was not intended to
>> >> >   handle gaps, we chose an artificially large end
>> >> >   address.  */
>> >> > -      end = (bfd_vma) 0x7ffffffffffffffUL;
>> >> > +      end = ~((bfd_vma)1 << 63);
>> >> >      }
>> >> >    break;
>> >
>> > Why not use "(bfd_vma) -1"?
>>
>> Well - why was the original value not 0xffffffffffffffff?
>
> I suspect the intent was to use the largest positive bfd_signed_vma,
> but Nick typoed the number of 'f' chars, and of course got it wrong
> for 32-bit.  "(bfd_vma) -1 >> 1" would have been right if that was
> his intention.  But the value used does not matter much.  Any value
> hugely larger than what is expected for a .gnu.build.attributes should
> work from what I can see of the code.
>
> I'm going to commit the following.
>
> * objcopy.c (merge_gnu_build_notes): Use (bfd_vma) -1 as
> "artificially large" end address.

Thanks!

Jan