* [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line
@ 2021-02-14 8:52 Tom de Vries
2021-02-14 19:10 ` Mark Wielaard
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2021-02-14 8:52 UTC (permalink / raw)
To: dwz, jakub; +Cc: Mark Wielaard
Hi,
When running the test-suite like this:
...
$ make clean; make; make check CC="gcc -gdwarf-5" CXX="g++ -gdwarf-5"
...
we run into:
...
FAIL: dwz/testsuite/dwz.tests/odr-struct.sh
...
The reason for the FAIL is that this DIE:
...
<1><115>: Abbrev Number: 2 (DW_TAG_structure_type)
<116> DW_AT_name : aaa
<11a> DW_AT_byte_size : 16
<11b> DW_AT_decl_file : 2
<11c> DW_AT_decl_line : 4
<11d> DW_AT_sibling : <0x13a>
...
with abbrev:
...
2 DW_TAG_structure_type [has children]
DW_AT_name DW_FORM_string
DW_AT_byte_size DW_FORM_data1
DW_AT_decl_file DW_FORM_data1
DW_AT_decl_line DW_FORM_data1
DW_AT_sibling DW_FORM_ref4
DW_AT value: 0 DW_FORM value: 0
...
is not recognized as a duplicate of DIE:
...
<1><1b9>: Abbrev Number: 2 (DW_TAG_structure_type)
<1ba> DW_AT_name : aaa
<1be> DW_AT_byte_size : 16
<1bf> DW_AT_decl_file : 2
<1c0> DW_AT_decl_line : 4
<1c0> DW_AT_sibling : <0x1dd>
...
with abbrev:
...
2 DW_TAG_structure_type [has children]
DW_AT_name DW_FORM_string
DW_AT_byte_size DW_FORM_data1
DW_AT_decl_file DW_FORM_data1
DW_AT_decl_line DW_FORM_implicit_const: 4
DW_AT_sibling DW_FORM_ref4
DW_AT value: 0 DW_FORM value: 0
...
due to the DW_AT_decl_line attribute having different forms, DW_FORM_data1 and
DW_FORM_implicit_const.
Fix this in checksum_die and die_eq_1 by adding dedicated handling for
DW_AT_decl_line, similar to how that's done for DW_AT_decl_file.
Any comments?
Thanks,
- Tom
Handle DW_FORM_implicit_const for DW_AT_decl_line
2021-02-14 Tom de Vries <tdevries@suse.de>
PR dwz/27400
* dwz.c (checksum_die, die_eq_1): Handle DW_FORM_implicit_const for
DW_AT_decl_line.
---
dwz.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 58 insertions(+), 3 deletions(-)
diff --git a/dwz.c b/dwz.c
index 992da77..971c402 100644
--- a/dwz.c
+++ b/dwz.c
@@ -3510,7 +3510,35 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
case DW_AT_call_line:
case DW_AT_call_column:
if (ignore_locus)
- handled = true;
+ {
+ handled = true;
+ break;
+ }
+ switch (form)
+ {
+ case DW_FORM_data1: value = read_8 (ptr); handled = true; break;
+ case DW_FORM_data2: value = read_16 (ptr); handled = true; break;
+ case DW_FORM_data4: value = read_32 (ptr); handled = true; break;
+ case DW_FORM_data8: value = read_64 (ptr); handled = true; break;
+ case DW_FORM_udata:
+ value = read_uleb128 (ptr); handled = true; break;
+ case DW_FORM_implicit_const:
+ value = t->values[i]; handled = true; break;
+ default:
+ error (0, 0, "%s: Unhandled %s for %s",
+ dso->filename, get_DW_FORM_str (form),
+ get_DW_AT_str (t->attr[i].attr));
+ return 1;
+ }
+ if (handled)
+ {
+ ptr = old_ptr;
+ s = t->attr[i].attr;
+ die->u.p1.die_hash
+ = iterative_hash_object (s, die->u.p1.die_hash);
+ die->u.p1.die_hash
+ = iterative_hash_object (value, die->u.p1.die_hash);
+ }
break;
default:
break;
@@ -4797,8 +4825,35 @@ die_eq_1 (dw_cu_ref cu1, dw_cu_ref cu2,
case DW_AT_call_line:
case DW_AT_call_column:
if (ignore_locus)
- old_ptr1 = NULL;
- break;
+ {
+ old_ptr1 = NULL;
+ break;
+ }
+ switch (form1)
+ {
+ case DW_FORM_data1: value1 = read_8 (ptr1); break;
+ case DW_FORM_data2: value1 = read_16 (ptr1); break;
+ case DW_FORM_data4: value1 = read_32 (ptr1); break;
+ case DW_FORM_data8: value1 = read_64 (ptr1); break;
+ case DW_FORM_udata: value1 = read_uleb128 (ptr1); break;
+ case DW_FORM_implicit_const: value1 = t1->values[i]; break;
+ default: abort ();
+ }
+ switch (form2)
+ {
+ case DW_FORM_data1: value2 = read_8 (ptr2); break;
+ case DW_FORM_data2: value2 = read_16 (ptr2); break;
+ case DW_FORM_data4: value2 = read_32 (ptr2); break;
+ case DW_FORM_data8: value2 = read_64 (ptr2); break;
+ case DW_FORM_udata: value2 = read_uleb128 (ptr2); break;
+ case DW_FORM_implicit_const: value2 = t2->values[j]; break;
+ default: abort ();
+ }
+ if (value1 != value2)
+ FAIL;
+ i++;
+ j++;
+ continue;
default:
break;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line
2021-02-14 8:52 [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line Tom de Vries
@ 2021-02-14 19:10 ` Mark Wielaard
2021-02-14 19:19 ` Jakub Jelinek
2021-02-14 21:23 ` Tom de Vries
0 siblings, 2 replies; 6+ messages in thread
From: Mark Wielaard @ 2021-02-14 19:10 UTC (permalink / raw)
To: Tom de Vries, dwz, jakub
Hi Tom,
On Sun, 2021-02-14 at 09:52 +0100, Tom de Vries wrote:
> due to the DW_AT_decl_line attribute having different forms,
> DW_FORM_data1 and
> DW_FORM_implicit_const.
>
> Fix this in checksum_die and die_eq_1 by adding dedicated handling for
> DW_AT_decl_line, similar to how that's done for DW_AT_decl_file.
>
> Any comments?
Yes, this makes sense. Like you said, we do something like this already
for decl/call_file. I think technically the die_eq_1 part could be put
under the same switch case. But maybe you find it clearer to do it
separately to mirror the checksum_die part?
Together with your DW_LANG_C_plus_plus_{03,11,14} patch it fixes the --
odr testsuite failures I was seeing.
As a follow up this might actually make sense for more attributes.
Where we know the attribute value is interpreted as an signed or
unsigned value (so we know what data[124] represents). This is actually
slightly tricky, because it isn't always immediately clear which
attribute values DWARF intends to be signed/unsigned, and it might even
differ per language (or whether or not an attribute is associated with
a [referenced] signed or unsigned type).
But the following seem to be always be unsigned values that might
qualify because they have unsigned constants assigned:
DW_AT_encoding
DW_AT_decimal_sign
DW_AT_endianity
DW_AT_accessibility
DW_AT_visibility
DW_AT_virtuality
DW_AT_language
DW_AT_address_class
DW_AT_identifier_case
DW_AT_calling_convention
DW_AT_inline
DW_AT_ordering
DW_AT_discr_list
DW_AT_defaulted
And the following qualify because they describe sizes, or counts, when
constant class:
DW_AT_byte_size
DW_AT_bit_size
DW_AT_count
DW_AT_digit_count
DW_AT_rank
DW_AT_string_length_bit_size
DW_AT_string_length_byte_size
DW_AT_alignment
As are these when constant class, indicating positive offsets
DW_AT_high_pc
DW_AT_start_scope
DW_AT_data_member_location
DW_AT_data_bit_offset
Cheers,
Mark
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line
2021-02-14 19:10 ` Mark Wielaard
@ 2021-02-14 19:19 ` Jakub Jelinek
2021-02-14 21:27 ` Tom de Vries
2021-02-14 21:23 ` Tom de Vries
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2021-02-14 19:19 UTC (permalink / raw)
To: Mark Wielaard; +Cc: Tom de Vries, dwz
On Sun, Feb 14, 2021 at 08:10:49PM +0100, Mark Wielaard wrote:
> Yes, this makes sense. Like you said, we do something like this already
> for decl/call_file. I think technically the die_eq_1 part could be put
> under the same switch case. But maybe you find it clearer to do it
> separately to mirror the checksum_die part?
Yeah, I'm wondering if we just shouldn't hash and compare all constant class
forms with the same value the same (ok, except DW_FORM_data16 which is too
large). For DW_FORM_data{1,2,4,8} we don't know if it is signed or
unsigned, so perhaps just treat values with the msb clear that way?
Or sure, if we can put in a list of attributes that always have unsigned
or always have signed constants, we can even improve that.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line
2021-02-14 19:19 ` Jakub Jelinek
@ 2021-02-14 21:27 ` Tom de Vries
2021-02-15 12:54 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2021-02-14 21:27 UTC (permalink / raw)
To: Jakub Jelinek, Mark Wielaard; +Cc: dwz
On 2/14/21 8:19 PM, Jakub Jelinek wrote:
> On Sun, Feb 14, 2021 at 08:10:49PM +0100, Mark Wielaard wrote:
>> Yes, this makes sense. Like you said, we do something like this already
>> for decl/call_file. I think technically the die_eq_1 part could be put
>> under the same switch case. But maybe you find it clearer to do it
>> separately to mirror the checksum_die part?
>
> Yeah, I'm wondering if we just shouldn't hash and compare all constant class
> forms with the same value the same (ok, except DW_FORM_data16 which is too
> large). For DW_FORM_data{1,2,4,8} we don't know if it is signed or
> unsigned, so perhaps just treat values with the msb clear that way?
> Or sure, if we can put in a list of attributes that always have unsigned
> or always have signed constants, we can even improve that.
Make sense to me.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line
2021-02-14 21:27 ` Tom de Vries
@ 2021-02-15 12:54 ` Tom de Vries
0 siblings, 0 replies; 6+ messages in thread
From: Tom de Vries @ 2021-02-15 12:54 UTC (permalink / raw)
To: Jakub Jelinek, Mark Wielaard; +Cc: dwz
On 2/14/21 10:27 PM, Tom de Vries wrote:
> On 2/14/21 8:19 PM, Jakub Jelinek wrote:
>> On Sun, Feb 14, 2021 at 08:10:49PM +0100, Mark Wielaard wrote:
>>> Yes, this makes sense. Like you said, we do something like this already
>>> for decl/call_file. I think technically the die_eq_1 part could be put
>>> under the same switch case. But maybe you find it clearer to do it
>>> separately to mirror the checksum_die part?
>>
>> Yeah, I'm wondering if we just shouldn't hash and compare all constant class
>> forms with the same value the same (ok, except DW_FORM_data16 which is too
>> large). For DW_FORM_data{1,2,4,8} we don't know if it is signed or
>> unsigned, so perhaps just treat values with the msb clear that way?
>> Or sure, if we can put in a list of attributes that always have unsigned
>> or always have signed constants, we can even improve that.
>
> Make sense to me.
Filed as PR27421 - Improve DW_FORM_implicit_const handling in
checksum_die/die_eq_1 (
https://sourceware.org/bugzilla/show_bug.cgi?id=27421 ).
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line
2021-02-14 19:10 ` Mark Wielaard
2021-02-14 19:19 ` Jakub Jelinek
@ 2021-02-14 21:23 ` Tom de Vries
1 sibling, 0 replies; 6+ messages in thread
From: Tom de Vries @ 2021-02-14 21:23 UTC (permalink / raw)
To: Mark Wielaard, dwz, jakub
On 2/14/21 8:10 PM, Mark Wielaard wrote:
> Hi Tom,
>
> On Sun, 2021-02-14 at 09:52 +0100, Tom de Vries wrote:
>> due to the DW_AT_decl_line attribute having different forms,
>> DW_FORM_data1 and
>> DW_FORM_implicit_const.
>>
>> Fix this in checksum_die and die_eq_1 by adding dedicated handling for
>> DW_AT_decl_line, similar to how that's done for DW_AT_decl_file.
>>
>> Any comments?
>
> Yes, this makes sense. Like you said, we do something like this already
> for decl/call_file. I think technically the die_eq_1 part could be put
> under the same switch case. But maybe you find it clearer to do it
> separately to mirror the checksum_die part?
>
Well, the first parts are similar, but after the "(value1 == 0) ^
(value2 == 0))" things diverge quite a bit. I agree there's room for
sharing, but I wonder whether perhaps that's better done using an inline
function.
> Together with your DW_LANG_C_plus_plus_{03,11,14} patch it fixes the --
> odr testsuite failures I was seeing.
>
Thanks for confirming. Committed.
> As a follow up this might actually make sense for more attributes.
Ack. I went down that road initially but felt things got too
complicated for a simple bug fix, so reverted to this more simple
approach, but agreed, a more generic solution would be better.
Thanks,
- Tom
> Where we know the attribute value is interpreted as an signed or
> unsigned value (so we know what data[124] represents). This is actually
> slightly tricky, because it isn't always immediately clear which
> attribute values DWARF intends to be signed/unsigned, and it might even
> differ per language (or whether or not an attribute is associated with
> a [referenced] signed or unsigned type).
>
> But the following seem to be always be unsigned values that might
> qualify because they have unsigned constants assigned:
>
> DW_AT_encoding
> DW_AT_decimal_sign
> DW_AT_endianity
> DW_AT_accessibility
> DW_AT_visibility
> DW_AT_virtuality
> DW_AT_language
> DW_AT_address_class
> DW_AT_identifier_case
> DW_AT_calling_convention
> DW_AT_inline
> DW_AT_ordering
> DW_AT_discr_list
> DW_AT_defaulted
>
> And the following qualify because they describe sizes, or counts, when
> constant class:
> DW_AT_byte_size
> DW_AT_bit_size
> DW_AT_count
> DW_AT_digit_count
> DW_AT_rank
> DW_AT_string_length_bit_size
> DW_AT_string_length_byte_size
> DW_AT_alignment
>
> As are these when constant class, indicating positive offsets
> DW_AT_high_pc
> DW_AT_start_scope
> DW_AT_data_member_location
> DW_AT_data_bit_offset
>
> Cheers,
>
> Mark
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-15 12:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-14 8:52 [PATCH] Handle DW_FORM_implicit_const for DW_AT_decl_line Tom de Vries
2021-02-14 19:10 ` Mark Wielaard
2021-02-14 19:19 ` Jakub Jelinek
2021-02-14 21:27 ` Tom de Vries
2021-02-15 12:54 ` Tom de Vries
2021-02-14 21:23 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).