[Bug default/26297] New: Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[Bug default/26297] New: Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

            Bug ID: 26297
           Summary: Possible misinterpretation of DW_AT_declaration via
                    DW_AT_specification
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida+abigail at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

Hi Dodji.

I was looking into libabigail's emission of anonymous declaration-only unions
and starting comparing what I saw there against what dwarfdump and pahole -aAE
reported. The pahole output didn't look entirely trustworthy.

dwarfdump tests/data/test-read-dwarf/libtest23.so

shows a DW_TAG_union with a DW_AT_declaration yes attribute. I thought "how's
that?" but spotted that it was referred to via a DW_AT_specification attribute
from another declaration.

A little web search found the discussion

https://gdb-patches.sourceware.narkive.com/PLB1ixbx/dw-at-specification-long-ago-gdb-change

which concluded that DW_AT_declaration found by following a DW_AT_specification
should be ignored.

I coded a naive change that plumbed through a "followed_spec" boolean through
bits of the DWARF reader and the results were quite startling.

Please take a look at this and let me know your thoughts.

https://github.com/myxoid/libabigail/commit/fb3b7302a9923b8ee7ca279cc9492207140d886d

It may be useful to switch some tests to --type-id-style hash and turn on
annotation to better understand the changes. 

Regards,
Giuliano.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/26297] Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

--- Comment #1 from Giuliano Procida <gprocida+abigail at google dot com> ---
I thought this might be related to
https://sourceware.org/bugzilla/show_bug.cgi?id=26182.

In a spot check (not on a public kernel), my patch reduces the overall count of
types appearing as both declaration-only / defined mixes by about 15%.

It also fixes a case where some drivers were defining the same struct (name)
differently and libabigail was confusing the two types.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/26297] Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at klomp dot org

--- Comment #2 from Mark Wielaard <mark at klomp dot org> ---
(In reply to Giuliano Procida from comment #0)
> which concluded that DW_AT_declaration found by following a
> DW_AT_specification should be ignored.
> 
> I coded a naive change that plumbed through a "followed_spec" boolean
> through bits of the DWARF reader and the results were quite startling.
> 
> Please take a look at this and let me know your thoughts.
> 
> https://github.com/myxoid/libabigail/commit/
> fb3b7302a9923b8ee7ca279cc9492207140d886d

I think your analysis is correct, when looking up whether a DIE has the
DW_AT_declaration flag set, it should not follow DW_AT_specification or
DW_AT_abstract_origin attributes.

The patch looks somewhat convoluted though.
Could we fix this by just changing die_is_declaration_only () ?

die_is_declaration_only () calls die_flag_attribute () which uses
die_flag_attribute () which calls dwarf_attr_integrate ().

dwarf_attr_integrate () is what follows the DW_AT_specification to lookup the
attribute.

So I would propose to have a version of die_flag_attribute () that just uses
dwarf_attr () to lookup the flag if the attribute is DW_AT_declaration.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/26297] Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

--- Comment #3 from Giuliano Procida <gprocida+abigail at google dot com> ---
Hi Dodji.

I think there are a few places where DW_AT_specification is followed
explicitly, not just in search of particular attributes, but I'm not too
familiar with the DWARF reader. I agree that the change as-is is not too nice
and I'll see what I can do improve things. Thanks for the pointers.

Essentially, we need make sure we call die_is_declaration_only only on "root"
DIEs and add some logic to prevent the DW_AT_specification following in the
DW_AT_declaration case. The first part isn't yet clear to me, but the second
part is.

I'll also look into the distinction between DW_AT_specification and
DW_AT_abstract_origin.

This change also reduces kernel ABIs by about 30% which would be great if
definitely correct.

Given the large diff to tests, I felt I should do a bit more investigation.

To reduce the churn, I cherry-picked my changes for to switch changes to hash
type ids for annotate and DWARF reader tests and eventually ended up with...

LANG=C git show --stat=10000 | cut -f2 -d ' ' | grep abi$ | while read x; do
diff --label a/$x --label b/$x -u <(git show HEAD~1:$x | sed -E -e "s;
(mangled-name|elf-symbol-id|filepath|line|column|is-declaration-only|declared-inline)='[^']*';;g"
| sort -u) <(git show HEAD:$x | sed -E -e "s;
(mangled-name|elf-symbol-id|filepath|line|column|is-declaration-only|declared-inline)='[^']*';;g"
-e 's/858845f7/ccf88c3b/g' | sort -u); done | sed -e '/<!--/d' -e /parameter/d
| less

as a way of getting an overview of what, if anything, had been removed or added
by the change. Essentially, all the generated ABIs looked about OK.

On the other hand, there is a big diff diff for one of the abipkgdiff tests.

tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt

Here the one important thing appears to be "SpiceWatch::worker" changing type
between the two library versions. I've got as far as dumping ABI XML at the
point the two corpora are compared in abipkgdiff, before and after my change,
but I only see class SpiceWatch as declaration-only.

I've not gone as far as trying dwarfdump directly as yet.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/26297] Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

--- Comment #4 from Giuliano Procida <gprocida+abigail at google dot com> ---
Some progress.

It's possible and easy to update die_is_declaration only to not follow links.
This makes some difference.

For the rest, passing through some state is still needed. However, my first
attempt definitely contained something which blew up the
tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
test. This may have been due to treating DW_AT_abstract_origin differently from
DW_AT_specification in one place.

I've rethought and reworked it. Instead of recording whether we have reached a
DIE via one or more links, I think it makes more sense to track the conjunction
of all the declaration-only states in a chain.

DIE1 --> DIE2 --> DIE3
decl     (no      decl
only      attr)   only

This should be treated as NOT declaration-only. Each time a link is followed,
it's either used for scope logic which doesn't care about this or there's a
recursive call to build_ir_node_from_die.

So, build_ir_node_from_die should take an is_declaration_only argument,
defaulted to true and (almost) immediately AND it with
die_is_declaration_only(die). This does leave much the same amount of plumbing
as before but it does make more logical sense.

WIP changes at https://github.com/myxoid/libabigail/commits/dwarf-follow-spec

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/26297] Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

--- Comment #5 from Giuliano Procida <gprocida+abigail at google dot com> ---
The latest tests/ diffs look good.

Some bits of ABI removed.

Plenty of cases where a struct with members is now no longer declaration-only
(success!)

A few empty structs (with size 8 bits) are no longer declaration-only. These
are things which in header files were defined as struct { }. dwarfdump confirms
the size as 1 byte. This is all as expected.
http://www.cantrip.org/emptyopt.html

I'll clean up the patches, prepare commit messages and rebase onto master
before sending for review.

Regards,
Giuliano.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/26297] Possible misinterpretation of DW_AT_declaration via DW_AT_specification

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26297

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from dodji at redhat dot com ---
This should now be fixed by this commit that got applied to the master branch
of the Git repository at
https://sourceware.org/git/?p=libabigail.git;a=commit;h=0bd4b93517f73a7423e0d1003c343440ccab5ed4.

Thanks!

-- 
You are receiving this mail because:
You are on the CC list for the bug.