Bug 27418 - DW_FORM_implicit_const handling in checksum_die missing attribute hashing
Summary: DW_FORM_implicit_const handling in checksum_die missing attribute hashing
Status: RESOLVED FIXED
Alias: None
Product: dwz
Classification: Unclassified
Component: default (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-14 21:33 UTC by Tom de Vries
Modified: 2021-02-15 13:20 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2021-02-14 21:33:32 UTC 
This is something that I don't get:
...
        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;
...

Normally, we have something like:
...
              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);
...
so we hash in both the attribute tag and the attribute value.

Why don't we hash in the attribute tag for DW_FORM_implicit_const?

This sound like a bug that makes hash collisions more likely.
Comment 1 Mark Wielaard 2021-02-14 22:30:28 UTC 
I think you are right. The bug seems to be the handled = true; That is not true, we haven't yet handled the attribute completely. We just added the value to the hash. The code should then continue so it ends up at the block at the end after the switch statement:

      if (!handled && die->die_ck_state != CK_BAD)
        {
          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 (old_ptr, ptr - old_ptr, die->u.p1.die_hash);
        }
Comment 2 Tom de Vries 2021-02-15 12:24:49 UTC 
(In reply to Mark Wielaard from comment #1)
> I think you are right. The bug seems to be the handled = true; That is not
> true, we haven't yet handled the attribute completely. We just added the
> value to the hash. The code should then continue so it ends up at the block
> at the end after the switch statement:
> 
>       if (!handled && die->die_ck_state != CK_BAD)
>         {
>           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 (old_ptr, ptr - old_ptr, die->u.p1.die_hash);
>         }

That does however revert the attribute-value order.  So I've fixed it differently: https://sourceware.org/git/?p=dwz.git;a=commit;h=29823e8858b824906d2630683032491125446961
Comment 3 Mark Wielaard 2021-02-15 13:20:03 UTC 
(In reply to Tom de Vries from comment #2)
> That does however revert the attribute-value order.  So I've fixed it
> differently:
> https://sourceware.org/git/?p=dwz.git;a=commit;
> h=29823e8858b824906d2630683032491125446961

Yes, keeping the attribute-value order in hashing makes sense. Your way is more consistent. Thanks.