Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-09-18 13:49, Ulrich Weigand wrote:
> Simon Marchi wrote:
> 
>> On 2017-09-05 20:20, Ulrich Weigand wrote:
>> >    size_t len = bit / TARGET_CHAR_BIT;
>> > -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
>> > -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
>> > +  gdb_assert (len * TARGET_CHAR_BIT >= floatformat->totalsize);
>> 
>> That looks funny now.  Is there a reason not to do
>> 
>>    gdb_assert (bit >= floatformat->totalsize);
>> 
>> directly?
> 
> Well, that would use different rounding ...   Of course, this only
> matters when floatformat->totalsize is not a multiple of 
> TARGET_CHAR_BIT,
> which doesn't happen for any of the floatformats GDB supports, so it
> is probably moot. Maybe we should instead use an assert to verify that
> floatformat->totalsize is in fact a multiple of TARGET_CHAR_BIT?

I indeed think we should add an assert that the type size in bits is a 
multiple of TARGET_CHAR_BIT.  But it should not be in 
verify_floatformat, because it's not specific to float.  In most calls 
to arch_type/init_type, we have that division that rounds down:

   t = init_type (objfile, TYPE_CODE_FLT, bit / TARGET_CHAR_BIT, name);

Let's say we try to add an integer with bit == 24 and TARGET_CHAR_BIT == 
16, we'll create a type with a size of 1 target byte, and things will 
likely break down the line.  Since this is where we do the division by 
TARGET_CHAR_BIT, and therefore assume (implicitly) that bit is a 
multiple of TARGET_CHAR_BIT, I think this is where the assert should be 
added.  To avoid adding them everywhere, we could make 
arch_type/init_type take a size in bits, and do the division and assert 
there.

As long as verify_floatformat is concerned, its job is to verify (IIUC) 
that the type you want to create with BIT bits has enough room to hold a 
float of that kind.  It doesn't have to know that we'll later divide 
that value by TARGET_CHAR_BIT.  So here I think asserting "bit >= 
floatformat->totalsize)" is fine.

I don't think floatformat->totalsize has any requirement to be a 
multiple of TARGET_CHAR_BIT.  For example, my hardware could have a 
float of 29 bits, but when stored in memory it uses 32 bits (just like 
on x86 long double is an 80 bit float stored in a 96 bit data type).  
Then, bit would be 32 and floatformat->totalsize would be 29.

Simon