[PATCH 2/3] [binutils/readelf] Add fixme in display_debug_str_offsets

[Tom de Vries]

On 5/15/24 15:02, Alan Modra wrote:
> On Wed, May 15, 2024 at 10:06:39AM +0200, Tom de Vries wrote:
>> On 5/15/24 09:37, Alan Modra wrote:
>>> On Tue, May 14, 2024 at 12:55:21PM +0200, Tom de Vries wrote:
>>>> While reading display_debug_str_offsets, I realized that the v4 split dwarf
>>>> handling bit doesn't assign entry_length, so it can be either 4 (32-bit dwarf)
>>>> or 8 (64-bit dwarf).  However, I think the cases for which it'll be 8 are
>>>> rare and odd enough to conclude that in practise we default to 32-bit
>>>> dwarf.
>>>>
>>>> As mentioned in the fission documentation [1], whether a .debug_str_offsets
>>>> section is 32-bit or 64-bit dwarf depends on the compilation unit header in
>>>> the .debug_info.dwo section, which is currently not inspected.
>>>>
>>>> For now:
>>>> - make an explicit choice for 32-bit dwarf by assiging 4 to entry_length, and
>>>> - add a fixme comment.
>>>>
>>>> [1] https://gcc.gnu.org/wiki/DebugFission
>>>> ---
>>>>    binutils/dwarf.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/binutils/dwarf.c b/binutils/dwarf.c
>>>> index 8125c64e1b6..f49989dc536 100644
>>>> --- a/binutils/dwarf.c
>>>> +++ b/binutils/dwarf.c
>>>> @@ -7971,6 +7971,14 @@ display_debug_str_offsets (struct dwarf_section *section,
>>>>    	  entries_end = end;
>>>>    	  debug_str_offsets_hdr_len = 0;
>>>> +	  /* FIXME: We assume 32-bit dwarf here.  According to
>>>> +	     https://gcc.gnu.org/wiki/DebugFission:
>>>> +	     The size of each entry is 4 bytes for DWARF-32 compilation units,
>>>> +	     or 8 bytes for DWARF-64 compilation units, as determined by the
>>>> +	     unit_length field of the compilation unit header in the
>>>> +	     .debug_info.dwo section.  */
>>>> +	  entry_length = 4;
>>>> +
>>>>    	  printf (_("    Length: %#" PRIx64 "\n"), length);
>>>>    	  printf (_("       Index   Offset [String]\n"));
>>>>    	}
>>>> -- 
>>>> 2.35.3
>>>
>>> I don't see the need for this patch.  If length is zero we already
>>> have entry_length of four.
>>>
>>
>> This follow-up demonstrator patch:
>> - adds an assert asserting that entry_length is four, and
>> - modifies the test-case to trigger it.
>>
>> Thanks,
>> - Tom
>>
>> diff --git a/binutils/dwarf.c b/binutils/dwarf.c
>> index cdf73a7bc50..586c87c3afd 100644
>> --- a/binutils/dwarf.c
>> +++ b/binutils/dwarf.c
>> @@ -7985,6 +7985,7 @@ display_debug_str_offsets
>>   	     or 8 bytes for DWARF-64 compilation units, as determined by the
>>   	     unit_length field of the compilation unit header in the
>>   	     .debug_info.dwo section.  */
>> +	  assert (entry_length == 4);
>>   	  entry_length = 4;
>>
>>   	  printf (_("    Length: %#" PRIx64 "\n"), length);
>> diff --git a/binutils/testsuite/binutils-all/readelf-debug-str-offsets-dw4.s
>> b/binutils/testsuite/binutils-all/read
>> elf-debug-str-offsets-dw4.s
>> index 1f16f4571e3..45f79be75f2 100644
>> --- a/binutils/testsuite/binutils-all/readelf-debug-str-offsets-dw4.s
>> +++ b/binutils/testsuite/binutils-all/readelf-debug-str-offsets-dw4.s
>> @@ -2,8 +2,8 @@
>>   	.asciz	"FIRST"
>>   	.asciz	"SECOND"
>>   	.section	.debug_str_offsets.dwo,"MS",%progbits,1
>> +	.4byte 0xffffffff
>>   	.4byte 0
>> -	.4byte 6
>>   	.section        .debug_macro.dwo,"e",%progbits
>>   	.2byte  0x4     /* DWARF macro version number.  */
>>   	.byte   0x0     /* Flags: 32-bit dwarf.  */
> 
> Sorry, that doesn't look like a realistic testcase to me.  An offset
> of 0xffffffff in 32-bit dwarf?
> 

I'm not claiming that this is a realistic test-case.

That's what I meant in the commit message with "rare and odd".

Thanks,
- Tom