Bug 25042

Summary: abidw aborts on clang compiled gdbm shared library
Product: libabigail Reporter: Ben Woodard <woodard>
Component: defaultAssignee: Dodji Seketeli <dodji>
Status: RESOLVED FIXED    
Severity: normal CC: libabigail, mark
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: problematic file

Description Ben Woodard 2019-09-27 22:52:38 UTC
Created attachment 12007 [details]
problematic file

current libabigail built from trunk as of 4fba6bea177ffa1981b8252f7f1967814b3ecf94 crashes on the attached binary.

$ ~/tmp/test/bin/abidw ./opt/spack/linux-fedora30-x86_64/clang-8.0.0.O0/gdbm-1.18.1-4l6xjbu74zu2aqu7gwrf3xzfcyun4abo/lib/libgdbm.so.6.0.0 
abidw: ../../src/abg-dwarf-reader.cc:10605: bool abigail::dwarf_reader::compare_dies_string_attribute_value(const Dwarf_Die*, const Dwarf_Die*, unsigned int, bool&): Assertion `__abg_cond__' failed.
Aborted (core dumped)

The cflags were: -g3 -gdwarf-5 -O0 my guess is that it has something to do with the DWARF5. I'll do a build using -gdwarf-4 for comparison.
Comment 1 Ben Woodard 2019-09-27 23:01:08 UTC
It is confirmed that the dwarf-4 version doesn't crash abidw.
Comment 2 Dodji Seketeli 2019-09-30 15:38:35 UTC
Ah, this looks like we need to support the DWARF forms DW_FORM_strx{1,4} from DWARF5.

I am on it.

Thanks for reporting this!
Comment 3 Mark Wielaard 2019-09-30 17:43:08 UTC
Note that elfutils libdw should already handle DW_FORM_strx forms since 0.171.

In general you shouldn't need to handle/check the actual DW_FORM an attribute is encoded with.

I see compare_dies_string_attribute_value () does, so it can do a pointer comparison instead of doing a full string comparison. But I think the logic there is broken, especially when dealing with split-dwarf DWO files.

How much "speedup" do you get from that "optimization"?
Comment 4 Dodji Seketeli 2019-10-01 08:16:59 UTC
"mark at klomp dot org" <sourceware-bugzilla@sourceware.org> writes:

> I see compare_dies_string_attribute_value () does, so it can do a pointer
> comparison instead of doing a full string comparison.

[...]

Right we really need that optimization because we do type de-duplication
at the DWARF level. 

> How much "speedup" do you get from that "optimization"?

String comparisons dominates the profile when doing type de-duplication.
I don't recall the speed-up right now (I've worked on tons of speed
optimizations left and right, all guided by careful profiling), but
suffice it to say that the string comparison was absolutely necessary to
make the speed of the de-duplication (at the DWARF level) be acceptable
at the time.  The de-duplication itself being a major size optimization.

This is of course in the context of huge C binaries like the linux
kernel.  I haven't implemented this for C++ binaries yet, but at some
point I think it'll be necessary to look into it as we still have bugs
open about libabigail being slow on huge C++ binaries.

> But I think the logic there is broken, especially when dealing with
> split-dwarf DWO files.

Well, libabigail doesn't yet support DWO files, so I wouldn't worry
about that at this point.

When we do support split-dwarf DWO files, then my understanding is that
type de-duplication would then be un-necessary because the
de-duplication would have been done by the DWARF emitter already, by
construction.  At that point, this optimization will be unnecessary,
hopefully.

But until then and for all the cases where we need to support non-DWO
files, I think this optimization will be necessary.  Anyhow, this
comparison speed-up is encapsulated at one place so when it becomes
unnecessary, it's easy to do away with.

So should we really worry about this?
Comment 5 Dodji Seketeli 2019-10-02 08:21:21 UTC
This should now be fixed by commits https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=commit;h=568dee18a1358e79047e94d10f7eee6f4632464c and https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=commit;h=1f53d1faa90f119d946f6fc7db9c09c2af767cb5.

The fix should be available in the upcoming 1.7 release of Libabigail.

Thank you for filling this issue.
Comment 6 Dodji Seketeli 2019-10-02 08:22:22 UTC
Closing this thus.