[2/3,binutils/readelf] Add fixme in display_debug_str_offsets
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Testing passed
|
Commit Message
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(+)
Comments
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.
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. */
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?
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
On Wed, May 15, 2024 at 04:18:58PM +0200, Tom de Vries wrote:
> On 5/15/24 15:02, Alan Modra wrote:
> > 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".
It isn't "rare and odd". Impossible to occur in any but fuzzed object
files is more like it. I definitely do not want to see an assertion
(resulting in bug reports) that can only occur with fuzzed objects,
and where failure of the assumption does not cause any bad behaviour.
By "bad behaviour" I mean
- segfaults and similar,
- buffer overflow,
- infinite looping,
- and maybe OOM.
On 5/16/24 02:14, Alan Modra wrote:
> On Wed, May 15, 2024 at 04:18:58PM +0200, Tom de Vries wrote:
>> On 5/15/24 15:02, Alan Modra wrote:
>>> 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".
>
> It isn't "rare and odd". Impossible to occur in any but fuzzed object
> files is more like it. I definitely do not want to see an assertion
> (resulting in bug reports) that can only occur with fuzzed objects,
> and where failure of the assumption does not cause any bad behaviour.
>
> By "bad behaviour" I mean
> - segfaults and similar,
> - buffer overflow,
> - infinite looping,
> - and maybe OOM.
>
I'm not proposing to add an assertion.
As mentioned, the patch containing the assert is a demonstrator patch.
The purpose of the proposed patch is to fixate and document the current
state of things (which needs fixing). If that's not acceptable, I can
open a PR to document it.
Thanks,
- Tom
@@ -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"));
}