public inbox for dwz@sourceware.org
 help / color / mirror / Atom feed
* [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: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

* 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

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).