Re: [PATCH] bfd/elf-hppa.h: Cleanup constants and comments

[Helge Deller <deller at gmx dot de>]

Hi Nick,

On 21.08.2018 16:30, Nick Clifton wrote:
>> Minor cleanups. Please commit if Ok.
> 
> Actually I have a couple of questions:
> 
>>  #if ARCH_SIZE == 64
>> -      hdr->sh_type = SHT_LOPROC + 1;
>> +      hdr->sh_type = SHT_LOPROC + SHT_PROGBITS;
> 
> The sh_type field is a single value, not a bitfield, so constructing a value
> like this is wrong.  A better solution, I would suggest, is to use the already
> defined value for this particular field.  IE:
> 
>       hdr->sh_type = SHT_PARISC_UNWIND;

Agreed. Much better!
 
>>  #else
>> -      hdr->sh_type = 1;
>> +      hdr->sh_type = SHT_PROGBITS;
>>  #endif
> 
> This is OK.

Ok.

>> -      /* I have no idea if this is really necessary or what it means.  */
>> +      /* Size of one unwind table entry is 16 (=2^4) bytes.  */
>>        hdr->sh_entsize = 4;
> 
> But if the unwind table entries are 16 bytes long, when why isn't the
> sh_entry field set to 16 ?  I think that the comment should explain
> why 4 is the correct value (or else change the sh_entsize field to 16,
> if that is the correct thing to do).

I'm think that 4 is the correct value here, because I think I read that somewhere once.
In that case I think "2 left-shifted by this value" should show the number of bytes per table line. 
But looking at all other usages of sh_entsize, this seems unlikely and everything is still unclear.
So, maybe we should simply leave the comment as is, until someone finds out?

Updated patch attached.
 
Helge

Attachment: p1
Description: Text document