[PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Julian Brown]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

Hi,

This patch fixes a problem with floatformat.c:get_field on 64-bit (on at 
least x86_64), when cross-assembling to arm-none-eabi. The line which reads:

   result = *(data + cur_byte) >> (-cur_bitshift);

was executed with cur_byte = -1 (start + len == 0 and order == 
floatformat_little), which happily segfaulted (during printing of FP 
immediates).

I had a little trouble following the logic of the function, so this is 
basically a rewrite. I hope that's OK.

Tested with cross (binutils) to arm-none-eabi from 
powerpc64-unknown-linux-gnu (i.e. big-endian) and 
x86_64-unknown-linux-gnu (i.e. little-endian). OK to apply?

Cheers,

Julian

ChangeLog:

     * floatformat.c (get_field): Fix segfault with little-endian word
     order on 64-bit hosts.
     (min): Move definition.

[-- Attachment #2: libiberty-getfield-2 --]
[-- Type: text/plain, Size: 3746 bytes --]

Index: libiberty/floatformat.c
===================================================================
RCS file: /cvs/src/src/libiberty/floatformat.c,v
retrieving revision 1.19.6.1
diff -c -p -r1.19.6.1 floatformat.c
*** libiberty/floatformat.c	24 Apr 2006 21:37:24 -0000	1.19.6.1
--- libiberty/floatformat.c	24 May 2006 12:51:20 -0000
*************** const struct floatformat floatformat_ia6
*** 249,301 ****
    floatformat_always_valid
  };
  \f
  /* Extract a field which starts at START and is LEN bits long.  DATA and
     TOTAL_LEN are the thing we are extracting it from, in byteorder ORDER.  */
  static unsigned long
  get_field (const unsigned char *data, enum floatformat_byteorders order,
             unsigned int total_len, unsigned int start, unsigned int len)
  {
!   unsigned long result;
    unsigned int cur_byte;
!   int cur_bitshift;
  
    /* Start at the least significant part of the field.  */
-   cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT;
    if (order == floatformat_little)
!     cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1;
!   cur_bitshift =
!     ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT;
!   result = *(data + cur_byte) >> (-cur_bitshift);
!   cur_bitshift += FLOATFORMAT_CHAR_BIT;
!   if (order == floatformat_little)
!     ++cur_byte;
    else
!     --cur_byte;
  
!   /* Move towards the most significant part of the field.  */
!   while ((unsigned int) cur_bitshift < len)
      {
!       if (len - cur_bitshift < FLOATFORMAT_CHAR_BIT)
! 	/* This is the last byte; zero out the bits which are not part of
! 	   this field.  */
! 	result |=
! 	  (*(data + cur_byte) & ((1 << (len - cur_bitshift)) - 1))
! 	    << cur_bitshift;
!       else
! 	result |= *(data + cur_byte) << cur_bitshift;
!       cur_bitshift += FLOATFORMAT_CHAR_BIT;
!       if (order == floatformat_little)
! 	++cur_byte;
!       else
! 	--cur_byte;
      }
!   return result;
  }
    
- #ifndef min
- #define min(a, b) ((a) < (b) ? (a) : (b))
- #endif
- 
  /* Convert from FMT to a double.
     FROM is the address of the extended float.
     Store the double in *TO.  */
--- 249,299 ----
    floatformat_always_valid
  };
  \f
+ 
+ #ifndef min
+ #define min(a, b) ((a) < (b) ? (a) : (b))
+ #endif
+ 
  /* Extract a field which starts at START and is LEN bits long.  DATA and
     TOTAL_LEN are the thing we are extracting it from, in byteorder ORDER.  */
  static unsigned long
  get_field (const unsigned char *data, enum floatformat_byteorders order,
             unsigned int total_len, unsigned int start, unsigned int len)
  {
!   unsigned long result = 0;
    unsigned int cur_byte;
!   int lo_bit, hi_bit, cur_bitshift = 0;
!   int nextbyte = (order == floatformat_little) ? 1 : -1;
! 
!   /* Start is in big-endian bit order!  Fix that first.  */
!   start = total_len - (start + len);
  
    /* Start at the least significant part of the field.  */
    if (order == floatformat_little)
!     cur_byte = start / FLOATFORMAT_CHAR_BIT;
    else
!     cur_byte = (total_len - start - 1) / FLOATFORMAT_CHAR_BIT;
  
!   lo_bit = start % FLOATFORMAT_CHAR_BIT;
!   hi_bit = min (lo_bit + len, FLOATFORMAT_CHAR_BIT);
!   
!   do
      {
!       unsigned int shifted = *(data + cur_byte) >> lo_bit;
!       unsigned int bits = hi_bit - lo_bit;
!       unsigned int mask = (1 << bits) - 1;
!       result |= (shifted & mask) << cur_bitshift;
!       len -= bits;
!       cur_bitshift += bits;
!       cur_byte += nextbyte;
!       lo_bit = 0;
!       hi_bit = min (len, FLOATFORMAT_CHAR_BIT);
      }
!   while (len != 0);
! 
!   return result & ((2 << (total_len - 1)) - 1);
  }
    
  /* Convert from FMT to a double.
     FROM is the address of the extended float.
     Store the double in *TO.  */

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Ian Lance Taylor]

Julian Brown <julian@codesourcery.com> writes:

> This patch fixes a problem with floatformat.c:get_field on 64-bit (on
> at least x86_64), when cross-assembling to arm-none-eabi. The line
> which reads:
> 
>    result = *(data + cur_byte) >> (-cur_bitshift);
> 
> was executed with cur_byte = -1 (start + len == 0 and order ==
> floatformat_little), which happily segfaulted (during printing of FP
> immediates).

I don't understand how start + len == 0 could ever be true.  What was
calling the function?  I note that put_field has the exact same
problem if start + len == 0.

> !   return result & ((2 << (total_len - 1)) - 1);

Why do you need to do this?  And if you do need to do it, why use 2?
Why not ((1 << total_len) - 1)?

Please compile the file as a standalone program with -DIEEE_DEBUG to
make sure those tests still work.  Ideally on both a big- and
little-endian system, if possible.

Ian

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Julian Brown]

Ian Lance Taylor wrote:
> Julian Brown <julian@codesourcery.com> writes:
> 
> 
>>This patch fixes a problem with floatformat.c:get_field on 64-bit (on
>>at least x86_64), when cross-assembling to arm-none-eabi. The line
>>which reads:
>>
>>   result = *(data + cur_byte) >> (-cur_bitshift);
>>
>>was executed with cur_byte = -1 (start + len == 0 and order ==
>>floatformat_little), which happily segfaulted (during printing of FP
>>immediates).
> 
> 
> I don't understand how start + len == 0 could ever be true.  What was
> calling the function?  I note that put_field has the exact same
> problem if start + len == 0.

Sorry, I botched my explanation a bit. I should have said:

   (start + len) / FLOATFORMAT_CHAR_BIT == 0

which is true when e.g. extracting the sign bit of a single-precision 
IEEE float -- in that case, start will be 0 and len will be 1 (with 
big-endian bit numbering used elsewhere in floatformat.c). The function 
is called by opcodes/arm-dis.c to print out the ARM Neon 
"quarter-precision" floating-point immediates.

>>!   return result & ((2 << (total_len - 1)) - 1);
> 
> 
> Why do you need to do this?  And if you do need to do it, why use 2?
> Why not ((1 << total_len) - 1)?

It was to attempt to maintain the original semantics of the function, as 
I understood them: the result is truncated to total_len after being 
built up <=8 bits at a time (though looking again, that might not have 
been the original intention at all, or at least not at that level of 
granularity... I suspect that bit should be removed).

The reason for using 2 << ... rather than 1 << .. was so that, e.g., the 
total_len == 32 case works properly. AIUI, shift amounts must be 
strictly less than the width of the operand, and the total_len==0 case 
isn't interesting. But that's irrelevant if I'm getting rid of that bit 
anyway :-)

So, I'll remove that bit and do the following...

> Please compile the file as a standalone program with -DIEEE_DEBUG to
> make sure those tests still work.  Ideally on both a big- and
> little-endian system, if possible.

Thanks,

Julian

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Ian Lance Taylor]

Julian Brown <julian@codesourcery.com> writes:

> So, I'll remove that bit and do the following...
> 
> > Please compile the file as a standalone program with -DIEEE_DEBUG to
> > make sure those tests still work.  Ideally on both a big- and
> > little-endian system, if possible.

This patch is OK with that change, assuming the tests pass.

Bonus points if you rewrite put_field.

Thanks.

Ian

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Julian Brown]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

Hi,

Ian Lance Taylor wrote:
>>>Please compile the file as a standalone program with -DIEEE_DEBUG to
>>>make sure those tests still work.  Ideally on both a big- and
>>>little-endian system, if possible.

I've done this (with a new put_field too), though the IEEE_DEBUG test 
needs explicitly changing to use floatformat_ieee_double_big not 
floatformat_ieee_double_little on a big-endian host, which wasn't 
immediately obvious...

> This patch is OK with that change, assuming the tests pass.
> 
> Bonus points if you rewrite put_field.

"put_field" indeed crashed too on x86_64 with IEEE_DEBUG defined, so 
I've rewritten it in a similar style. Just to be sure, is this still OK 
(for gcc and/or binutils)?

Cheers,

Julian

ChangeLog (libiberty):

     * floatformat.c (get_field): Fix segfault with little-endian word
     order on 64-bit hosts.
     (put_field): Likewise.
     (min): Move definition.

[-- Attachment #2: libiberty-getfield-5 --]
[-- Type: text/plain, Size: 6266 bytes --]

Index: floatformat.c
===================================================================
RCS file: /cvs/src/src/libiberty/floatformat.c,v
retrieving revision 1.20
diff -c -p -r1.20 floatformat.c
*** floatformat.c	24 Apr 2006 21:34:41 -0000	1.20
--- floatformat.c	15 Jun 2006 22:02:54 -0000
*************** const struct floatformat floatformat_ia6
*** 249,301 ****
    floatformat_always_valid
  };
  \f
  /* Extract a field which starts at START and is LEN bits long.  DATA and
     TOTAL_LEN are the thing we are extracting it from, in byteorder ORDER.  */
  static unsigned long
  get_field (const unsigned char *data, enum floatformat_byteorders order,
             unsigned int total_len, unsigned int start, unsigned int len)
  {
!   unsigned long result;
    unsigned int cur_byte;
!   int cur_bitshift;
  
    /* Start at the least significant part of the field.  */
-   cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT;
    if (order == floatformat_little)
!     cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1;
!   cur_bitshift =
!     ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT;
!   result = *(data + cur_byte) >> (-cur_bitshift);
!   cur_bitshift += FLOATFORMAT_CHAR_BIT;
!   if (order == floatformat_little)
!     ++cur_byte;
    else
!     --cur_byte;
  
!   /* Move towards the most significant part of the field.  */
!   while ((unsigned int) cur_bitshift < len)
      {
!       if (len - cur_bitshift < FLOATFORMAT_CHAR_BIT)
! 	/* This is the last byte; zero out the bits which are not part of
! 	   this field.  */
! 	result |=
! 	  (*(data + cur_byte) & ((1 << (len - cur_bitshift)) - 1))
! 	    << cur_bitshift;
!       else
! 	result |= *(data + cur_byte) << cur_bitshift;
!       cur_bitshift += FLOATFORMAT_CHAR_BIT;
!       if (order == floatformat_little)
! 	++cur_byte;
!       else
! 	--cur_byte;
      }
    return result;
  }
    
- #ifndef min
- #define min(a, b) ((a) < (b) ? (a) : (b))
- #endif
- 
  /* Convert from FMT to a double.
     FROM is the address of the extended float.
     Store the double in *TO.  */
--- 249,299 ----
    floatformat_always_valid
  };
  \f
+ 
+ #ifndef min
+ #define min(a, b) ((a) < (b) ? (a) : (b))
+ #endif
+ 
  /* Extract a field which starts at START and is LEN bits long.  DATA and
     TOTAL_LEN are the thing we are extracting it from, in byteorder ORDER.  */
  static unsigned long
  get_field (const unsigned char *data, enum floatformat_byteorders order,
             unsigned int total_len, unsigned int start, unsigned int len)
  {
!   unsigned long result = 0;
    unsigned int cur_byte;
!   int lo_bit, hi_bit, cur_bitshift = 0;
!   int nextbyte = (order == floatformat_little) ? 1 : -1;
! 
!   /* Start is in big-endian bit order!  Fix that first.  */
!   start = total_len - (start + len);
  
    /* Start at the least significant part of the field.  */
    if (order == floatformat_little)
!     cur_byte = start / FLOATFORMAT_CHAR_BIT;
    else
!     cur_byte = (total_len - start - 1) / FLOATFORMAT_CHAR_BIT;
  
!   lo_bit = start % FLOATFORMAT_CHAR_BIT;
!   hi_bit = min (lo_bit + len, FLOATFORMAT_CHAR_BIT);
!   
!   do
      {
!       unsigned int shifted = *(data + cur_byte) >> lo_bit;
!       unsigned int bits = hi_bit - lo_bit;
!       unsigned int mask = (1 << bits) - 1;
!       result |= (shifted & mask) << cur_bitshift;
!       len -= bits;
!       cur_bitshift += bits;
!       cur_byte += nextbyte;
!       lo_bit = 0;
!       hi_bit = min (len, FLOATFORMAT_CHAR_BIT);
      }
+   while (len != 0);
+ 
    return result;
  }
    
  /* Convert from FMT to a double.
     FROM is the address of the extended float.
     Store the double in *TO.  */
*************** put_field (unsigned char *data, enum flo
*** 428,470 ****
             unsigned long stuff_to_put)
  {
    unsigned int cur_byte;
!   int cur_bitshift;
  
    /* Start at the least significant part of the field.  */
-   cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT;
-   if (order == floatformat_little)
-     cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1;
-   cur_bitshift =
-     ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT;
-   *(data + cur_byte) &=
-     ~(((1 << ((start + len) % FLOATFORMAT_CHAR_BIT)) - 1) << (-cur_bitshift));
-   *(data + cur_byte) |=
-     (stuff_to_put & ((1 << FLOATFORMAT_CHAR_BIT) - 1)) << (-cur_bitshift);
-   cur_bitshift += FLOATFORMAT_CHAR_BIT;
    if (order == floatformat_little)
!     ++cur_byte;
    else
!     --cur_byte;
  
!   /* Move towards the most significant part of the field.  */
!   while ((unsigned int) cur_bitshift < len)
      {
!       if (len - cur_bitshift < FLOATFORMAT_CHAR_BIT)
! 	{
! 	  /* This is the last byte.  */
! 	  *(data + cur_byte) &=
! 	    ~((1 << (len - cur_bitshift)) - 1);
! 	  *(data + cur_byte) |= (stuff_to_put >> cur_bitshift);
! 	}
!       else
! 	*(data + cur_byte) = ((stuff_to_put >> cur_bitshift)
! 			      & ((1 << FLOATFORMAT_CHAR_BIT) - 1));
!       cur_bitshift += FLOATFORMAT_CHAR_BIT;
!       if (order == floatformat_little)
! 	++cur_byte;
!       else
! 	--cur_byte;
      }
  }
  
  /* The converse: convert the double *FROM to an extended float
--- 426,459 ----
             unsigned long stuff_to_put)
  {
    unsigned int cur_byte;
!   int lo_bit, hi_bit;
!   int nextbyte = (order == floatformat_little) ? 1 : -1;
! 
!   /* Start is in big-endian bit order!  Fix that first.  */
!   start = total_len - (start + len);
  
    /* Start at the least significant part of the field.  */
    if (order == floatformat_little)
!     cur_byte = start / FLOATFORMAT_CHAR_BIT;
    else
!     cur_byte = (total_len - start - 1) / FLOATFORMAT_CHAR_BIT;
  
!   lo_bit = start % FLOATFORMAT_CHAR_BIT;
!   hi_bit = min (lo_bit + len, FLOATFORMAT_CHAR_BIT);
!   
!   do
      {
!       unsigned char *byte_ptr = data + cur_byte;
!       unsigned int bits = hi_bit - lo_bit;
!       unsigned int mask = ((1 << bits) - 1) << lo_bit;
!       *byte_ptr = (*byte_ptr & ~mask) | ((stuff_to_put << lo_bit) & mask);
!       stuff_to_put >>= bits;
!       len -= bits;
!       cur_byte += nextbyte;
!       lo_bit = 0;
!       hi_bit = min (len, FLOATFORMAT_CHAR_BIT);
      }
+   while (len != 0);
  }
  
  /* The converse: convert the double *FROM to an extended float

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Mark Mitchell]

Julian Brown wrote:

> "put_field" indeed crashed too on x86_64 with IEEE_DEBUG defined, so
> I've rewritten it in a similar style. Just to be sure, is this still OK
> (for gcc and/or binutils)?

Julian, did you ever get a reply to this?  (It doesn't look to me as
though the patch has gone in, though perhaps I'm just not seeing it.)
Ian, might you be able to take a look at this last version, if you've
not already?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Ian Lance Taylor]

Julian Brown <julian@codesourcery.com> writes:

> ChangeLog (libiberty):
> 
>      * floatformat.c (get_field): Fix segfault with little-endian word
>      order on 64-bit hosts.
>      (put_field): Likewise.
>      (min): Move definition.

This is OK.

Sorry, it fell far enough back in my mail queue that I was no longer
seeing it.

Thanks.

Ian

Re: [PATCH, libiberty] Fix segfault in floatformat.c:get_field on 64-bit hosts

[Julian Brown]

Ian Lance Taylor wrote:
> Julian Brown <julian@codesourcery.com> writes:
> 
>> ChangeLog (libiberty):
>>
>>      * floatformat.c (get_field): Fix segfault with little-endian word
>>      order on 64-bit hosts.
>>      (put_field): Likewise.
>>      (min): Move definition.
> 
> This is OK.
> 
> Sorry, it fell far enough back in my mail queue that I was no longer
> seeing it.

This patch is now committed in both GCC's and binutils' copies of 
libiberty. Sorry I've been so slow at getting around to committing it! 
(This fixes some long-standing test failures on HEAD).

Cheers,

Julian