[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[woodard at redhat dot com]

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

Ben Woodard <woodard at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |woodard at redhat dot com

--- Comment #8 from Ben Woodard <woodard at redhat dot com> ---
One of the projects that I'm going to move to subsequently will be comparing
binaries created by different compilers. So please do not close this quite yet.
What I would suggest:

1) When abidiff opens up a binary that doesn't have debug information emit a
warning.
2) Try to reproduce the problem with libraries which have debug information.
If the problem doesn't exist the warning in item 1 should suffice.
If the problem still exists carefully look at the DWARF to understand the
problem. Then depending on what we figure out, we decide to take action.

What I'm ultimately looking for is for:
$ abidiff clang-elf-file.so gcc-elf-file.so
$ echo $?
0

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[dodji at redhat dot com]

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

--- Comment #15 from dodji at redhat dot com ---
(In reply to Jakub Jelinek from comment #4)
> Bet that is because abidiff looks at the debug info, which in this case
> doesn't reflect the bogus size change either.  In GCC, the size of the
> symbol is never changed, because it is an ABI change, we just insert padding
> after the symbol.

I gave this some more thoughts.

For a function, the size of the underlying symbol is the size taken by the
function in the .text segment.

So a change of that size does not indicate an ABI change.

For a global variable, however, I think it's different.  In that case, the size
of the symbol is the size of the storage occupied by the variable.  In theory
that size should match the size of the type of that variable.  And a change in
that size is an ABI change.

So I think we should detect symbol size changes for variables, only.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[m.ostapenko at partner dot samsung.com]

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

--- Comment #5 from Maxim Ostapenko <m.ostapenko at partner dot samsung.com> ---
Indeed, this might be a clang's debug info bug. Consider:

max@max:/tmp$ cat libfoo.c 
#ifdef LONG
char buggy_symbol[10];
#else
char buggy_symbol[5];
#endif

max@max:/tmp$ gcc libfoo.c -shared -fpic -o libfoo.so -g 
max@max:/tmp$ gcc libfoo.c -shared -fpic -o libfoo2.so -g -DLONG 
max@max:/tmp$ ~/install/libabigail/bin/abidiff   libfoo.so libfoo2.so
Functions changes summary: 0 Removed, 0 Changed, 0 Added function
Variables changes summary: 0 Removed, 1 Changed, 0 Added variable

1 Changed variable:

  [C]'char buggy_symbol[5]' was changed to 'char buggy_symbol[10]':
    type of variable changed:
     type name changed from 'char[5]' to 'char[10]'
     array type size changed from 40 to 80 bits:
     array type subrange 1 changed length from 5 to 10


This looks perfectly fine. But for clang the situation is different:

max@max:/tmp$ clang libfoo.c -shared -fpic -o libfoo.so -g
max@max:/tmp$ clang libfoo.c -shared -fpic -o libfoo2.so -g -DLONG 
max@max:/tmp$ ~/install/libabigail/bin/abidiff   libfoo.so libfoo2.so

So, should I close this as invalid because abidiff relies on a debug info here
and this is a clang's (?) debug info issue? Or maybe there is some other way to
detect this ABI breakage (e.g. by looking into symtab for symbol's size)?

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[jakub at redhat dot com]

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

--- Comment #11 from Jakub Jelinek <jakub at redhat dot com> ---
(In reply to Ben Woodard from comment #10)
> So I would say that you uncovered 2 related bugs.
> 1) The fact that libabigail doesn't handle DW_AT_count
> 2) Logic that makes them equivalent in cases where it can be assumed that
> the lower bound is zero. This is needed to be able to correctly compare the
> ABI between ELF files generated by different compilers.

DW_AT_count is not equivalent to DW_AT_upper_bound if DW_AT_lower_bound is 0
(implicitly or explicitly), lower bound has to be 1 instead for that.
But generally, if either DW_AT_lower_bound is (implicit or explicit) constant
and DW_AT_count or DW_AT_upper_bound is constant too, it is trivial to convert
one to the other.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[m.ostapenko at partner dot samsung.com]

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

--- Comment #12 from Maxim Ostapenko <m.ostapenko at partner dot samsung.com> ---
(In reply to Ben Woodard from comment #10)
> (In reply to Maxim Ostapenko from comment #9)
> 
> So I would say that you uncovered 2 related bugs.
> 1) The fact that libabigail doesn't handle DW_AT_count
> 2) Logic that makes them equivalent in cases where it can be assumed that
> the lower bound is zero. This is needed to be able to correctly compare the
> ABI between ELF files generated by different compilers.

Yes, trivial fix works fine for array symbols.

However, abidiff doesn't detect the case, where we have a simple variable
symbol (e.g. like buggy_symbol from my first comment) that suddenly changed
it's size (LLVM + ASan does such thing) due to changed link options
(-fsanitize=address in my case), because debug info actually doesn't hold
information about symbol size.
All we have are it's type and the size of this type (both GCC and Clang).

Although GCC doesn't do such awful things, I think it would be nice to detect
such kind of bugs.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

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

--- Comment #16 from dodji at redhat dot com ---
This issue should now be fixed by the two commits below that landed in the
master branch:

https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=commit;h=ba980025fba9b554fdb0b7116d91c902183a2211

and

https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=commit;h=5d24cf87d7683f7dc620ddfa278efed30a137409

With those two commits applied, here is what I am getting, on the two binaries
attached to this bug (which I renamed when I added them to the test suite of
libabigail):

$ abidiff test34-pr19173-libfoo.so test34-pr19173-libfoo2.so
Functions changes summary: 0 Removed, 0 Changed, 0 Added function
Variables changes summary: 0 Removed, 1 Changed, 0 Added variable

1 Changed variable:

  [C]'long int buggy_symbol' was changed:
    size of symbol (in bytes) changed from 8 to 64

$ 

On the second example that involves arrays and the DW_AT_count DWARF attribute,
here is what the patched libabigail gives me:

$ abidiff test35-pr19173-libfoo-long-clang.so
test35-pr19173-libfoo-long-clang2.so 
Functions changes summary: 0 Removed, 0 Changed, 0 Added function
Variables changes summary: 0 Removed, 1 Changed, 0 Added variable

1 Changed variable:

  [C]'char buggy_symbol[5]' was changed to 'char buggy_symbol[10]':
    size of symbol (in bytes) changed from 5 to 10
    type of variable changed:
     type name changed from 'char[5]' to 'char[10]'
     array type size changed from 40 to 80 bits:
     array type subrange 1 changed length from 5 to 10

$ 

Maxim, thank you a lot for taking time to report this issue.  It did make a
real difference, and that is appreciated!

And a big thank also goes to all of you, folks, who took time to comment on
this issue.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[woodard at redhat dot com]

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

--- Comment #10 from Ben Woodard <woodard at redhat dot com> ---
(In reply to Maxim Ostapenko from comment #9)
> GCC uses DW_AT_upper_bound to represent array upper bound while Clang uses
> DW_AT_count. Looking to source code of libabigail, I noticed that
> build_array_type function from abg-dwarf-reader.cc handles
> DW_AT_upper_bound, but not DW_AT_count. Is this intentional?

I can almost guarantee the answer to this is "no".

The standard says:

The subrange entry may have the attributes DW_AT_lower_bound and
DW_AT_upper_bound to specify, respectively, the lower and upper bound values of
the subrange. The DW_AT_upper_bound attribute may be replaced by a DW_AT_count
attribute, whose value describes the number of elements in the subrange rather
than the value of the last element. The value of each of these attributes is
determined as described in Section 2.19.

So I would say that you uncovered 2 related bugs.
1) The fact that libabigail doesn't handle DW_AT_count
2) Logic that makes them equivalent in cases where it can be assumed that the
lower bound is zero. This is needed to be able to correctly compare the ABI
between ELF files generated by different compilers.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[m.ostapenko at partner dot samsung.com]

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

--- Comment #1 from Maxim Ostapenko <m.ostapenko at partner dot samsung.com> ---
Created attachment 8742
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8742&action=edit
libfoo2.so

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

[Bug default/19173] New: Abidiff doesn't detect symbol size change in library.

[m.ostapenko at partner dot samsung.com]

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

            Bug ID: 19173
           Summary: Abidiff doesn't detect symbol size change in library.
           Product: libabigail
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: m.ostapenko at partner dot samsung.com
                CC: libabigail at sourceware dot org
  Target Milestone: ---
              Host: x86_64-pc-linux-gnu
            Target: x86_64-pc-linux-gnu
             Build: x86_64-pc-linux-gnu

Created attachment 8741
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8741&action=edit
libfoo.so

Hi Dodji.

I was playing with ASan in LLVM and noticed, that it can implicitly change
global variable symbol size (that is seems to be an ABI breakage). However,
abidiff didn't report this change:

max@max:/tmp$ cat libfoo.c 
long buggy_symbol = 4;
long foo (long *p) { return *p; }

max@max:/tmp$ clang libfoo.c -shared -fpic -o libfoo.so -g 

max@max:/tmp$ readelf -s libfoo.so | grep buggy_symbol
     7: 0000000000201030     8 OBJECT  GLOBAL DEFAULT   21 buggy_symbol
    49: 0000000000201030     8 OBJECT  GLOBAL DEFAULT   21 buggy_symbol

max@max:/tmp$ clang libfoo.c -shared -fpic -o libfoo2.so -g -fsanitize=address

max@max:/tmp$ readelf -s libfoo2.so | grep buggy_symbol
    12: 0000000000201080    64 OBJECT  GLOBAL DEFAULT   23 buggy_symbol
    56: 0000000000201080    64 OBJECT  GLOBAL DEFAULT   23 buggy_symbol

Is this an abidiff's bug (should it report such an issue)?

I'm attaching libfoo.so and libfoo2.so.
Thanks!

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[y.gribov at samsung dot com]

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

--- Comment #6 from Yury Gribov <y.gribov at samsung dot com> ---
Max, what if debug info is missing? Does abidiff still report bug (based on
.dynsym info)?

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[jakub at redhat dot com]

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

--- Comment #4 from Jakub Jelinek <jakub at redhat dot com> ---
Bet that is because abidiff looks at the debug info, which in this case doesn't
reflect the bogus size change either.  In GCC, the size of the symbol is never
changed, because it is an ABI change, we just insert padding after the symbol.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[mjw at redhat dot com]

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

Mark Wielaard <mjw at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mjw at redhat dot com

--- Comment #14 from Mark Wielaard <mjw at redhat dot com> ---
Note that elfutils libdw comes with this helper function (which for array types
does know about the default lower bound of the language, DW_AT_lower_bound,
DW_AT_upper_bound or DW_AT_count and the DW_AT_byte_stride):

/* Compute the byte-size of a type DIE according to DWARF rules.
   For most types, this is just DW_AT_byte_size.
   For DW_TAG_array_type it can apply much more complex rules.  */
extern int dwarf_aggregate_size (Dwarf_Die *die, Dwarf_Word *size);

But that doesn't handle the ELF symbol type, just the Dwarf size.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[m.ostapenko at partner dot samsung.com]

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

--- Comment #9 from Maxim Ostapenko <m.ostapenko at partner dot samsung.com> ---
(In reply to Ben Woodard from comment #8)

> If the problem still exists carefully look at the DWARF to understand the
> problem. Then depending on what we figure out, we decide to take action.

Hm, I've compared contents of .debug_info section for clang generated binary
and GCC generated one:

$ cat test.c

int buggy[1234];

and noticed this difference in debug info:

<<<<<<<<<<<<<<< test-clang.debug
.....
 <2><38>: Abbrev Number: 4 (DW_TAG_subrange_type)
    <39>   DW_AT_type        : <0x47>   
    <3d>   DW_AT_count       : 1234   <=========================

================================
.....
 <2><26>: Abbrev Number: 3 (DW_TAG_subrange_type)
    <27>   DW_AT_type        : <0x2e>   
    <2b>   DW_AT_upper_bound : 1233   <==========================

<<<<<<<<<<<<<<< test-gcc.debug

GCC uses DW_AT_upper_bound to represent array upper bound while Clang uses
DW_AT_count. Looking to source code of libabigail, I noticed that
build_array_type function from abg-dwarf-reader.cc handles DW_AT_upper_bound,
but not DW_AT_count. Is this intentional?

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[m.ostapenko at partner dot samsung.com]

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

--- Comment #7 from Maxim Ostapenko <m.ostapenko at partner dot samsung.com> ---
(In reply to Yury Gribov from comment #6)
> Max, what if debug info is missing? Does abidiff still report bug (based on
> .dynsym info)?

Nope, removing debug info removes the abidiff's report as well.

max@max:/tmp$ gcc libfoo.c -shared -fpic -o libfoo2.so  -DLONG 
max@max:/tmp$ gcc libfoo.c -shared -fpic -o libfoo.so  
max@max:/tmp$ ~/install/libabigail/bin/abidiff libfoo.so libfoo2.so

Actually, libabigail documentation states that the tool relies on debug info
quite hardly, so I'm not sure that running abidiff on stripped libraries makes
sense. On the other hand, the bug might be quite common and reporting it might
be desirable even if debug info is missing (just as for added/removed
functions).

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[dodji at seketeli dot org]

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

dodji at seketeli dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dodji at seketeli dot org

--- Comment #13 from dodji at seketeli dot org ---
Yes, I think there are two issues that need to be addressed in libabigail here.

1/ Libabigail doesn't take symbols' size into account, and I think it should.

2/ Libabigail doesn't handle the DW_AT_count attribute -- presumably because
when the support for array types was added, we used GCC as the debug info
producer, and it doesn't emit that attribute.  Note that there are others
arrays-related attributes that libabigail doesn't handle yet.

So I guess 1/ and 2/ ought to be addressed in two distinct patches.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[y.gribov at samsung dot com]

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

Yury Gribov <y.gribov at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |y.gribov at samsung dot com

--- Comment #2 from Yury Gribov <y.gribov at samsung dot com> ---
I think it's indeed an ABI change because symbol size is used during generation
of copy relocation in main executable. If library increases symbol size but
copy-relocated symbol size remains the same, you may run into nasty runtime
errors.

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

[Bug default/19173] Abidiff doesn't detect symbol size change in library.

[y.gribov at samsung dot com]

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

Yury Gribov <y.gribov at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at redhat dot com

--- Comment #3 from Yury Gribov <y.gribov at samsung dot com> ---
Summoning Jakub.

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