Created attachment 13137 [details] tarball with g++ gcc ar nm built with DWARF5 .debug_info and .debug_line for i in g++ gcc ar nm; do cp -af $i.orig $i; done; rm -f gcc.dwz; ./dwz -m gcc.dwz g++ gcc ar nm dwz: dwz.c:15837: alt_clear_dups: Assertion `child->die_dup == NULL' failed. Aborted (core dumped)
It doesn't help with this case. But reviewing the code I found that there was at least one place where we didn't hash the DW_FORM_implicit_const value in checksum_die meaning that two DIEs which only differed in an attribute (which we didn't already handle specially) using a DW_FORM_implicit_const with a different value (maybe a DW_AT_byte_size) could possibly hash to the same value. diff --git a/dwz.c b/dwz.c index 308bcba..e556dfb 100644 --- a/dwz.c +++ b/dwz.c @@ -3580,7 +3580,11 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die) ptr += ptr_size; break; case DW_FORM_flag_present: + break; case DW_FORM_implicit_const: + if (! handled) + die->u.p1.die_hash + = iterative_hash_object (t->values[i], die->u.p1.die_hash); break; case DW_FORM_flag: case DW_FORM_data1:
Apparently the #27212 fix also fixes the Assertion failure.
(In reply to Mark Wielaard from comment #1) > It doesn't help with this case. But reviewing the code I found that there > was at least one place where we didn't hash the DW_FORM_implicit_const value > in checksum_die meaning that two DIEs which only differed in an attribute > (which we didn't already handle specially) using a DW_FORM_implicit_const > with a different value (maybe a DW_AT_byte_size) could possibly hash to the > same value. > > diff --git a/dwz.c b/dwz.c > index 308bcba..e556dfb 100644 > --- a/dwz.c > +++ b/dwz.c > @@ -3580,7 +3580,11 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref > top_die, dw_die_ref die) > ptr += ptr_size; > break; > case DW_FORM_flag_present: > + break; > case DW_FORM_implicit_const: > + if (! handled) > + die->u.p1.die_hash > + = iterative_hash_object (t->values[i], die->u.p1.die_hash); > break; > case DW_FORM_flag: > case DW_FORM_data1: As for this patch, I agree that something should be done about it, but wonder if it shouldn't be if (!handled && die->die_ck_state != CK_BAD) { handled = true; die->u.p1.die_hash = iterative_hash_object (t->values[i], die->u.p1.die_hash); }
Another thing I'm worried about, where in die_eq_1 we compare the DW_FORM_implicit_const values? I wonder about: case DW_FORM_flag_present: break; case DW_FORM_implicit_const: if ((!ignore_locus || old_ptr1) && t1->values[i] != t2->values[j]) FAIL; break; in die_eq_1.
So: diff --git a/dwz.c b/dwz.c index 308bcba..a6ea5ec 100644 --- a/dwz.c +++ b/dwz.c @@ -3580,7 +3580,14 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die) ptr += ptr_size; break; case DW_FORM_flag_present: + break; case DW_FORM_implicit_const: + if (!handled && die->die_ck_state != CK_BAD) + { + handled = true; + die->u.p1.die_hash + = iterative_hash_object (t->values[i], die->u.p1.die_hash); + } break; case DW_FORM_flag: case DW_FORM_data1: @@ -4837,7 +4844,11 @@ die_eq_1 (dw_cu_ref cu1, dw_cu_ref cu2, ptr2 += ptr_size; break; case DW_FORM_flag_present: + break; case DW_FORM_implicit_const: + if ((!ignore_locus || old_ptr1) + && t1->values[i] != t2->values[j]) + FAIL; break; case DW_FORM_flag: case DW_FORM_data1: @@ -10456,19 +10467,18 @@ build_abbrevs_for_die (htab_t h, dw_cu_ref cu, dw_die_ref die, case DW_FORM_data4: value = read_32 (ptr); break; case DW_FORM_data8: value = read_64 (ptr); break; case DW_FORM_udata: value = read_uleb128 (ptr); break; - case DW_FORM_implicit_const: break; + case DW_FORM_implicit_const: + value = reft->values[i]; + break; default: error (0, 0, "Unhandled %s for %s", get_DW_FORM_str (form), get_DW_AT_str (reft->attr[i].attr)); return 1; } - /* Note that the value is only used for calculating the - DIE size and possibly change form. Doesn't change the - implicit_const from or value. */ + value = line_htab_lookup (refcu, value); if (form != DW_FORM_implicit_const) { - value = line_htab_lookup (refcu, value); if (value <= 0xff) { form = DW_FORM_data1; @@ -10488,7 +10498,7 @@ build_abbrevs_for_die (htab_t h, dw_cu_ref cu, dw_die_ref die, t->attr[j].attr = reft->attr[i].attr; t->attr[j].form = form; if (form == DW_FORM_implicit_const) - t->values[j] = reft->values[i]; + t->values[j] = value; j++; continue; } @@ -12088,10 +12098,8 @@ write_die (unsigned char *ptr, dw_cu_ref cu, dw_die_ref die, update = true; break; case DW_FORM_implicit_const: - /* Negative means, already transformed. */ - if (reft->values[i] >= 0) - update = true; - value = reft->values[i]; + /* DW_FORM_implicit_const should have been updated + already when computing abbrevs. */ break; default: abort (); } @@ -12104,9 +12112,6 @@ write_die (unsigned char *ptr, dw_cu_ref cu, dw_die_ref die, case DW_FORM_data2: write_16 (ptr, value); break; case DW_FORM_data4: write_32 (ptr, value); break; case DW_FORM_udata: write_uleb128 (ptr, value); break; - case DW_FORM_implicit_const: - reft->values[i] = -value; /* Note, negated. */ - break; default: abort (); } } as the full patch?
(In reply to Jakub Jelinek from comment #3) > As for this patch, I agree that something should be done about it, > but wonder if it shouldn't be > if (!handled && die->die_ck_state != CK_BAD) > { > handled = true; > die->u.p1.die_hash > = iterative_hash_object (t->values[i], die->u.p1.die_hash); > } Yes, checking for CK_BAD is correct. Thanks.
(In reply to Jakub Jelinek from comment #4) > Another thing I'm worried about, where in die_eq_1 we compare the > DW_FORM_implicit_const values? > I wonder about: > case DW_FORM_flag_present: > break; > case DW_FORM_implicit_const: > if ((!ignore_locus || old_ptr1) > && t1->values[i] != t2->values[j]) > FAIL; > break; > in die_eq_1. Yes, I was just looking at that one. It would make checksum_die and die_eq agree with each other. Looks correct.
author Jakub Jelinek <jakub@redhat.com> Thu, 21 Jan 2021 07:43:08 +0000 (08:43 +0100) committer Jakub Jelinek <jakub@redhat.com> Thu, 21 Jan 2021 07:43:08 +0000 (08:43 +0100) commit 47af8da7c8533924e96d1dbd97625b6a4dc35855 tree 4a924fd99e9ee98dcc4125836309dff06de39b19 tree parent 40d5efd4edc52a7d2ed02f8400a9beb129053271 commit | diff Assorted DW_FORM_implicit_const fixes The problem is I think that write_abbrev is always called before write_info, and the remapping of the DW_AT_decl_file/DW_AT_call_file DW_FORM_implicit_const is done only in write_die which is called either recursively or from write_info/write_types. Which means the DW_FORM_implicit_const value encoded in the abbrev was never updated. Other fixes include hashing the DW_FORM_implicit_const value in checksum_die and more importantly comparing the values in die_eq_1. 2021-01-21 Jakub Jelinek <jakub@redhat.com> PR dwz/27212 PR dwz/27213 * dwz.c (checksum_die): For DW_FORM_implicit_const hash t->values[i]. (die_eq_1): For DW_FORM_implicit_const compare t?->values[?]. (build_abbrevs_for_die): For DW_FORM_implicit_const on DW_AT_*_file call line_htab_lookup too and store the result into t->values[j]. Otherwise, handle even file ids larger than 32-bit. (write_die): For DW_FORM_implicit_const on DW_AT_*_file just j++ and continue, don't call line_htab_lookup nor adjust anything. Simplify. (alt_clear_dups): Fix function comment.
Fixed by the commit from comment #8.