Re: 'src/abg-dwarf-reader.cc:compare_dies_string_attribute_value' optimization

[Giuliano Procida <gprocida@google.com>]

I started looking at this today but I've run out of time. I'll be able to
take another look in about a week.

Checks fail (various types are renumbered in XML output) starting at
commit 57b1c714.

I haven't tried this on kernel or framework libraries yet, but I know there
are plenty of typedefs in both.

Regards,
Giuliano

On Mon, 11 Apr 2022 at 16:18, Dodji Seketeli <dodji@seketeli.org> wrote:

> Hello Thomas and Giuliano,
>
> [...]
>
>
> Thomas Schwinge <thomas@codesourcery.com> a écrit:
>
> [...]
>
> >> OK.  Then:
> >>
> >>     (gdb) print l_attr.valp && r_attr.valp
> >>     $8 = true
> >>     (gdb) print *l_attr.valp == *r_attr.valp
> >>     $9 = true
> >>
> >> This now does 'return true;'!
> >>
> >> Given 'unsigned char *valp', what this only checks is that one byte at
> >> 'l_attr.valp' equals one byte at 'r_attr.valp' -- which evidently isn't
> >> sufficient to demonstrate that 'l_attr' and 'r_attr' do match.
> Therefore
> >> it seems that this optimization is not correct?
> >>
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > Looks like you've found one bug. It has compared addresses and moved on
> to
> > test data.
> >
> > It could do strcmp at this point, assuming these things are
> null-terminated
> > strings, which is the case for DW_FORM_strp but is not for other things.
> >
> > But there is another bug. I've instrumented the code and found cases (in
> > the test suite) where the fast path returns false, but the slow path
> > returns true. Both sides are DW_FORM_strx1. The data are not strings but
> > indexes and that the same string appears at different indexes.
>
> I agree.  This is a bug.
>
> I tried to fix and indeed, hell did break lose as I realized there were
> several other issues across the pipeline (including several issues that
> I have long wanted to address) that needed so that this one fits in
> correctly.
>
> I came up with a patch set that lies in the branch 'fix-dwarf-str-cmp',
> at
>
> https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/fix-dwarf-str-cmp
> .
>
> The tip of that patch set addresses this issue specifically:
>
> https://sourceware.org/git/?p=libabigail.git;a=commit;h=3d277a9cc05873cf4aeb97273d585e0b07af917d
> .
>
> But for that fix to be applied, we need the whole patch set that fixes
> several other issues left and right.
>
> Giuliano & Thomas, could you please test that branch on your specific
> environments?  If it works OK, then I can consider posting the patch-set
> properly to the list.  If it doesn't, at least I'd wouldn't have broken
> the world without notice ;-)
>
> Thanks.
>
> --
>                 Dodji
>