* [PATCH] ir,dwarf-reader: Peel const-qualifier from const this pointers
@ 2024-03-06 13:09 Dodji Seketeli
2024-03-06 14:51 ` [PATCH] ir, dwarf-reader: " Ben Woodard
0 siblings, 1 reply; 4+ messages in thread
From: Dodji Seketeli @ 2024-03-06 13:09 UTC (permalink / raw)
To: libabigail
[-- Attachment #1: Type: text/plain, Size: 9144 bytes --]
Hello,
In DWARF, it appears that the this pointer of a concrete instance of
method is sometimes represented as a const pointer, whereas the this
pointer of the abstract instance (or interface) of that same method
has the this pointer represented as a non-const pointer. That
difference often causes some spurious const-ness change reports on the
this pointer when doing comparisons.
This patch thus trims of the const-qualifier off of the this pointer
of methods.
* include/abg-fwd.h (is_const_qualified_type)
(peel_const_qualified_type): Declare ...
* src/abg-ir.cc (is_const_qualified_type)
(peel_const_qualified_type): ... new functions.
* src/abg-dwarf-reader.cc (build_function_type): Trim the const
qualifier off of the this pointer representation if present.
* tests/data/test-annotate/test1.abi: Adjust.
* tests/data/test-annotate/test14-pr18893.so.abi: Likewise.
* tests/data/test-annotate/test15-pr18892.so.abi: Likewise.
* tests/data/test-annotate/test17-pr19027.so.abi: Likewise.
* tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Likewise.
* tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Likewise.
* tests/data/test-annotate/test2.so.abi: Likewise.
* tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Likewise.
* tests/data/test-annotate/test6.so.abi: Likewise.
* tests/data/test-annotate/test8-qualified-this-pointer.so.abi:
Likewise.
* tests/data/test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1.abi:
Likewise.
* tests/data/test-diff-dwarf/test0-report.txt: Likewise.
* tests/data/test-diff-dwarf/test28-vtable-changes-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test29-vtable-changes-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test30-vtable-changes-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test36-ppc64-aliases-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test41-PR20476-hidden-report-0.txt:
Likewise.
* tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt:
Likewise.
* tests/data/test-diff-dwarf/test5-report.txt: Likewise.
* tests/data/test-diff-dwarf/test8-report.txt: Likewise.
* tests/data/test-diff-filter/test0-report.txt: Likewise.
* tests/data/test-diff-filter/test01-report.txt: Likewise.
* tests/data/test-diff-filter/test10-report.txt: Likewise.
* tests/data/test-diff-filter/test13-report.txt: Likewise.
* tests/data/test-diff-filter/test2-report.txt: Likewise.
* tests/data/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt:
Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt:
Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt:
Likewise.
* tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt:
Likewise.
* tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt:
Likewise.
* tests/data/test-diff-filter/test41-report-0.txt: Likewise.
* tests/data/test-diff-filter/test9-report.txt: Likewise.
* tests/data/test-diff-pkg/libsigc++-2.0-0c2a_2.4.0-1_amd64--libsigc++-2.0-0v5_2.4.1-1ubuntu2_amd64-report-0.txt:
Likewise.
* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt:
Likewise.
* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt:
Likewise.
* tests/data/test-diff-suppr/test24-soname-report-1.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-10.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-12.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-14.txt: Likewise.
* tests/data/test-diff-suppr/test24-soname-report-16.txt:
Likewise.
* tests/data/test-diff-suppr/test24-soname-report-4.txt: Likewise.
* tests/data/test-diff-suppr/test31-report-1.txt: Likewise.
* tests/data/test-read-dwarf/test1.abi: Likewise.
* tests/data/test-read-dwarf/test1.hash.abi: Likewise.
* tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Likewise.
* tests/data/test-read-dwarf/test11-pr18828.so.abi: Likewise.
* tests/data/test-read-dwarf/test14-pr18893.so.abi: Likewise.
* tests/data/test-read-dwarf/test15-pr18892.so.abi: Likewise.
* tests/data/test-read-dwarf/test16-pr18904.so.abi: Likewise.
* tests/data/test-read-dwarf/test17-pr19027.so.abi: Likewise.
* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
Likewise.
* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
Likewise.
* tests/data/test-read-dwarf/test2.so.abi: Likewise.
* tests/data/test-read-dwarf/test2.so.hash.abi: Likewise.
* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
Likewise.
* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
Likewise.
* tests/data/test-read-dwarf/test6.so.abi: Likewise.
* tests/data/test-read-dwarf/test6.so.hash.abi: Likewise.
* tests/data/test-read-dwarf/test8-qualified-this-pointer.so.abi:
Likewise.
* tests/data/test-read-dwarf/test8-qualified-this-pointer.so.hash.abi:
Likewise.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applying to master.
---
include/abg-fwd.h | 9 +
src/abg-dwarf-reader.cc | 17 +
src/abg-ir.cc | 49 +
tests/data/test-annotate/test1.abi | 38 +-
.../data/test-annotate/test14-pr18893.so.abi | 5184 +--
.../data/test-annotate/test15-pr18892.so.abi | 10940 +++---
.../data/test-annotate/test17-pr19027.so.abi | 11084 +++---
...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 11952 +++---
...19-pr19023-libtcmalloc_and_profiler.so.abi | 10178 ++---
tests/data/test-annotate/test2.so.abi | 22 +-
...st20-pr19025-libvtkParallelCore-6.1.so.abi | 13572 +++----
tests/data/test-annotate/test6.so.abi | 6 +-
.../test8-qualified-this-pointer.so.abi | 6 +-
.../test0-pr19026-libvtkIOSQL-6.1.so.1.abi | 17627 ++-------
tests/data/test-diff-dwarf/test0-report.txt | 32 +-
.../test28-vtable-changes-report-0.txt | 3 -
.../test29-vtable-changes-report-0.txt | 20 +-
.../test30-vtable-changes-report-0.txt | 20 +-
.../test31-vtable-changes-report-0.txt | 20 +-
.../test36-ppc64-aliases-report-0.txt | 21 +-
.../test41-PR20476-hidden-report-0.txt | 54 +-
.../test42-PR21296-clanggcc-report0.txt | 9 +-
tests/data/test-diff-dwarf/test5-report.txt | 11 +-
tests/data/test-diff-dwarf/test8-report.txt | 38 +-
tests/data/test-diff-filter/test0-report.txt | 36 +-
tests/data/test-diff-filter/test01-report.txt | 36 +-
tests/data/test-diff-filter/test10-report.txt | 11 +-
tests/data/test-diff-filter/test13-report.txt | 23 +-
tests/data/test-diff-filter/test2-report.txt | 11 +-
...t-and-filtered-children-nodes-report-1.txt | 10 +-
.../test30-pr18904-rvalueref-report0.txt | 230 +-
.../test30-pr18904-rvalueref-report1.txt | 230 +-
.../test30-pr18904-rvalueref-report2.txt | 230 +-
.../test35-pr18754-no-added-syms-report-0.txt | 230 +-
.../data/test-diff-filter/test41-report-0.txt | 87 +-
tests/data/test-diff-filter/test9-report.txt | 11 +-
...-2.0-0v5_2.4.1-1ubuntu2_amd64-report-0.txt | 82 +-
...bb-4.3-3.20141204.fc23.x86_64-report-0.txt | 369 +-
...bb-4.3-3.20141204.fc23.x86_64-report-1.txt | 99 +-
.../test24-soname-report-1.txt | 13 +-
.../test24-soname-report-10.txt | 13 +-
.../test24-soname-report-12.txt | 13 +-
.../test24-soname-report-14.txt | 13 +-
.../test24-soname-report-16.txt | 13 +-
.../test24-soname-report-4.txt | 13 +-
.../data/test-diff-suppr/test31-report-1.txt | 11 +-
tests/data/test-read-dwarf/test1.abi | 28 +-
tests/data/test-read-dwarf/test1.hash.abi | 10 +-
.../test-read-dwarf/test10-pr18818-gcc.so.abi | 2966 +-
.../test-read-dwarf/test11-pr18828.so.abi | 654 +-
.../test-read-dwarf/test14-pr18893.so.abi | 4744 +--
.../test-read-dwarf/test15-pr18892.so.abi | 10402 ++---
.../test-read-dwarf/test16-pr18904.so.abi | 11286 +++---
.../test-read-dwarf/test17-pr19027.so.abi | 9810 ++---
...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 10790 ++---
...19-pr19023-libtcmalloc_and_profiler.so.abi | 8844 ++---
tests/data/test-read-dwarf/test2.so.abi | 14 +-
tests/data/test-read-dwarf/test2.so.hash.abi | 8 +-
...st20-pr19025-libvtkParallelCore-6.1.so.abi | 12218 +++---
.../test22-pr19097-libstdc++.so.6.0.17.so.abi | 32770 ++++++++--------
tests/data/test-read-dwarf/test6.so.abi | 4 +-
tests/data/test-read-dwarf/test6.so.hash.abi | 2 +-
.../test8-qualified-this-pointer.so.abi | 4 +-
.../test8-qualified-this-pointer.so.hash.abi | 2 +-
64 files changed, 89021 insertions(+), 98231 deletions(-)
The patch is too big for the mailing list so I am attaching it in a
gzip'ed format.
Cheers,
[-- Attachment #2: 0001-ir-dwarf-reader-Peel-const-qualifier-from-const-this.patch.gz --]
[-- Type: application/gzip, Size: 1781272 bytes --]
[-- Attachment #3: Type: text/plain, Size: 14 bytes --]
--
Dodji
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ir, dwarf-reader: Peel const-qualifier from const this pointers
2024-03-06 13:09 [PATCH] ir,dwarf-reader: Peel const-qualifier from const this pointers Dodji Seketeli
@ 2024-03-06 14:51 ` Ben Woodard
0 siblings, 0 replies; 4+ messages in thread
From: Ben Woodard @ 2024-03-06 14:51 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail
[-- Attachment #1: Type: text/plain, Size: 10477 bytes --]
I believe that I have a bug open on that one. Are you addressing it or did you independently find the problem? This is also something that causes false positive ABI problems between different toolchains.
The question for me though, is this the “right” fix? I remember reasoning through the DWARF and wondering if a “this” pointer really should be const indicating that it could be a bug in the compilers when it is not. This makes me wonder if peeling the const pointer is the right solution or if it is just expedient given the current state of the compilers.
-ben
> On Mar 6, 2024, at 5:18 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>
> Hello,
>
> In DWARF, it appears that the this pointer of a concrete instance of
> method is sometimes represented as a const pointer, whereas the this
> pointer of the abstract instance (or interface) of that same method
> has the this pointer represented as a non-const pointer. That
> difference often causes some spurious const-ness change reports on the
> this pointer when doing comparisons.
>
> This patch thus trims of the const-qualifier off of the this pointer
> of methods.
>
> * include/abg-fwd.h (is_const_qualified_type)
> (peel_const_qualified_type): Declare ...
> * src/abg-ir.cc (is_const_qualified_type)
> (peel_const_qualified_type): ... new functions.
> * src/abg-dwarf-reader.cc (build_function_type): Trim the const
> qualifier off of the this pointer representation if present.
> * tests/data/test-annotate/test1.abi: Adjust.
> * tests/data/test-annotate/test14-pr18893.so.abi: Likewise.
> * tests/data/test-annotate/test15-pr18892.so.abi: Likewise.
> * tests/data/test-annotate/test17-pr19027.so.abi: Likewise.
> * tests/data/test-annotate/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
> Likewise.
> * tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
> Likewise.
> * tests/data/test-annotate/test2.so.abi: Likewise.
> * tests/data/test-annotate/test20-pr19025-libvtkParallelCore-6.1.so.abi:
> Likewise.
> * tests/data/test-annotate/test6.so.abi: Likewise.
> * tests/data/test-annotate/test8-qualified-this-pointer.so.abi:
> Likewise.
> * tests/data/test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1.abi:
> Likewise.
> * tests/data/test-diff-dwarf/test0-report.txt: Likewise.
> * tests/data/test-diff-dwarf/test28-vtable-changes-report-0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test29-vtable-changes-report-0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test30-vtable-changes-report-0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test31-vtable-changes-report-0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test36-ppc64-aliases-report-0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test41-PR20476-hidden-report-0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt:
> Likewise.
> * tests/data/test-diff-dwarf/test5-report.txt: Likewise.
> * tests/data/test-diff-dwarf/test8-report.txt: Likewise.
> * tests/data/test-diff-filter/test0-report.txt: Likewise.
> * tests/data/test-diff-filter/test01-report.txt: Likewise.
> * tests/data/test-diff-filter/test10-report.txt: Likewise.
> * tests/data/test-diff-filter/test13-report.txt: Likewise.
> * tests/data/test-diff-filter/test2-report.txt: Likewise.
> * tests/data/test-diff-filter/test28-redundant-and-filtered-children-nodes-report-1.txt:
> Likewise.
> * tests/data/test-diff-filter/test30-pr18904-rvalueref-report0.txt:
> Likewise.
> * tests/data/test-diff-filter/test30-pr18904-rvalueref-report1.txt:
> Likewise.
> * tests/data/test-diff-filter/test30-pr18904-rvalueref-report2.txt:
> Likewise.
> * tests/data/test-diff-filter/test35-pr18754-no-added-syms-report-0.txt:
> Likewise.
> * tests/data/test-diff-filter/test41-report-0.txt: Likewise.
> * tests/data/test-diff-filter/test9-report.txt: Likewise.
> * tests/data/test-diff-pkg/libsigc++-2.0-0c2a_2.4.0-1_amd64--libsigc++-2.0-0v5_2.4.1-1ubuntu2_amd64-report-0.txt:
> Likewise.
> * tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt:
> Likewise.
> * tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt:
> Likewise.
> * tests/data/test-diff-suppr/test24-soname-report-1.txt: Likewise.
> * tests/data/test-diff-suppr/test24-soname-report-10.txt: Likewise.
> * tests/data/test-diff-suppr/test24-soname-report-12.txt: Likewise.
> * tests/data/test-diff-suppr/test24-soname-report-14.txt: Likewise.
> * tests/data/test-diff-suppr/test24-soname-report-16.txt:
> Likewise.
> * tests/data/test-diff-suppr/test24-soname-report-4.txt: Likewise.
> * tests/data/test-diff-suppr/test31-report-1.txt: Likewise.
> * tests/data/test-read-dwarf/test1.abi: Likewise.
> * tests/data/test-read-dwarf/test1.hash.abi: Likewise.
> * tests/data/test-read-dwarf/test10-pr18818-gcc.so.abi: Likewise.
> * tests/data/test-read-dwarf/test11-pr18828.so.abi: Likewise.
> * tests/data/test-read-dwarf/test14-pr18893.so.abi: Likewise.
> * tests/data/test-read-dwarf/test15-pr18892.so.abi: Likewise.
> * tests/data/test-read-dwarf/test16-pr18904.so.abi: Likewise.
> * tests/data/test-read-dwarf/test17-pr19027.so.abi: Likewise.
> * tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
> Likewise.
> * tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
> Likewise.
> * tests/data/test-read-dwarf/test2.so.abi: Likewise.
> * tests/data/test-read-dwarf/test2.so.hash.abi: Likewise.
> * tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
> Likewise.
> * tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
> Likewise.
> * tests/data/test-read-dwarf/test6.so.abi: Likewise.
> * tests/data/test-read-dwarf/test6.so.hash.abi: Likewise.
> * tests/data/test-read-dwarf/test8-qualified-this-pointer.so.abi:
> Likewise.
> * tests/data/test-read-dwarf/test8-qualified-this-pointer.so.hash.abi:
> Likewise.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> Applying to master.
> ---
> include/abg-fwd.h | 9 +
> src/abg-dwarf-reader.cc | 17 +
> src/abg-ir.cc | 49 +
> tests/data/test-annotate/test1.abi | 38 +-
> .../data/test-annotate/test14-pr18893.so.abi | 5184 +--
> .../data/test-annotate/test15-pr18892.so.abi | 10940 +++---
> .../data/test-annotate/test17-pr19027.so.abi | 11084 +++---
> ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 11952 +++---
> ...19-pr19023-libtcmalloc_and_profiler.so.abi | 10178 ++---
> tests/data/test-annotate/test2.so.abi | 22 +-
> ...st20-pr19025-libvtkParallelCore-6.1.so.abi | 13572 +++----
> tests/data/test-annotate/test6.so.abi | 6 +-
> .../test8-qualified-this-pointer.so.abi | 6 +-
> .../test0-pr19026-libvtkIOSQL-6.1.so.1.abi | 17627 ++-------
> tests/data/test-diff-dwarf/test0-report.txt | 32 +-
> .../test28-vtable-changes-report-0.txt | 3 -
> .../test29-vtable-changes-report-0.txt | 20 +-
> .../test30-vtable-changes-report-0.txt | 20 +-
> .../test31-vtable-changes-report-0.txt | 20 +-
> .../test36-ppc64-aliases-report-0.txt | 21 +-
> .../test41-PR20476-hidden-report-0.txt | 54 +-
> .../test42-PR21296-clanggcc-report0.txt | 9 +-
> tests/data/test-diff-dwarf/test5-report.txt | 11 +-
> tests/data/test-diff-dwarf/test8-report.txt | 38 +-
> tests/data/test-diff-filter/test0-report.txt | 36 +-
> tests/data/test-diff-filter/test01-report.txt | 36 +-
> tests/data/test-diff-filter/test10-report.txt | 11 +-
> tests/data/test-diff-filter/test13-report.txt | 23 +-
> tests/data/test-diff-filter/test2-report.txt | 11 +-
> ...t-and-filtered-children-nodes-report-1.txt | 10 +-
> .../test30-pr18904-rvalueref-report0.txt | 230 +-
> .../test30-pr18904-rvalueref-report1.txt | 230 +-
> .../test30-pr18904-rvalueref-report2.txt | 230 +-
> .../test35-pr18754-no-added-syms-report-0.txt | 230 +-
> .../data/test-diff-filter/test41-report-0.txt | 87 +-
> tests/data/test-diff-filter/test9-report.txt | 11 +-
> ...-2.0-0v5_2.4.1-1ubuntu2_amd64-report-0.txt | 82 +-
> ...bb-4.3-3.20141204.fc23.x86_64-report-0.txt | 369 +-
> ...bb-4.3-3.20141204.fc23.x86_64-report-1.txt | 99 +-
> .../test24-soname-report-1.txt | 13 +-
> .../test24-soname-report-10.txt | 13 +-
> .../test24-soname-report-12.txt | 13 +-
> .../test24-soname-report-14.txt | 13 +-
> .../test24-soname-report-16.txt | 13 +-
> .../test24-soname-report-4.txt | 13 +-
> .../data/test-diff-suppr/test31-report-1.txt | 11 +-
> tests/data/test-read-dwarf/test1.abi | 28 +-
> tests/data/test-read-dwarf/test1.hash.abi | 10 +-
> .../test-read-dwarf/test10-pr18818-gcc.so.abi | 2966 +-
> .../test-read-dwarf/test11-pr18828.so.abi | 654 +-
> .../test-read-dwarf/test14-pr18893.so.abi | 4744 +--
> .../test-read-dwarf/test15-pr18892.so.abi | 10402 ++---
> .../test-read-dwarf/test16-pr18904.so.abi | 11286 +++---
> .../test-read-dwarf/test17-pr19027.so.abi | 9810 ++---
> ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi | 10790 ++---
> ...19-pr19023-libtcmalloc_and_profiler.so.abi | 8844 ++---
> tests/data/test-read-dwarf/test2.so.abi | 14 +-
> tests/data/test-read-dwarf/test2.so.hash.abi | 8 +-
> ...st20-pr19025-libvtkParallelCore-6.1.so.abi | 12218 +++---
> .../test22-pr19097-libstdc++.so.6.0.17.so.abi | 32770 ++++++++--------
> tests/data/test-read-dwarf/test6.so.abi | 4 +-
> tests/data/test-read-dwarf/test6.so.hash.abi | 2 +-
> .../test8-qualified-this-pointer.so.abi | 4 +-
> .../test8-qualified-this-pointer.so.hash.abi | 2 +-
> 64 files changed, 89021 insertions(+), 98231 deletions(-)
>
> The patch is too big for the mailing list so I am attaching it in a
> gzip'ed format.
>
>
> Cheers,
>
[-- Attachment #2: 0001-ir-dwarf-reader-Peel-const-qualifier-from-const-this.patch.gz --]
[-- Type: application/x-gzip, Size: 1781272 bytes --]
[-- Attachment #3: Type: text/plain, Size: 27 bytes --]
>
>
> --
> Dodji
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ir, dwarf-reader: Peel const-qualifier from const this pointers
@ 2024-03-06 15:15 Dodji Seketeli
2024-03-06 15:43 ` Ben Woodard
0 siblings, 1 reply; 4+ messages in thread
From: Dodji Seketeli @ 2024-03-06 15:15 UTC (permalink / raw)
To: Ben Woodard; +Cc: Dodji Seketeli, libabigail
Hello Ben,
Ben Woodard <woodard@redhat.com> a écrit:
> I believe that I have a bug open on that one.
Yes, there are a series of bugs of yours related to that, I think. I
need to re-test them to see where we stand today. A number of other
things have been fixed since then, so I might need to take another
view at the whole thing.
> Are you addressing it or did you independently find the problem?
I was looking at something else (abidb), but I had your issues in mind
too when I stumbled upon this.
> This is also something that causes false positive ABI problems between
> different toolchains.
Exactly. This patch reduces the distance between the output of g++ and
clang, as shown by the change that the patch has on the test result
tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt.
> The question for me though, is this the “right” fix? I remember
> reasoning through the DWARF and wondering if a “this” pointer really
> should be const indicating that it could be a bug in the compilers
> when it is not. This makes me wonder if peeling the const pointer is
> the right solution or if it is just expedient given the current state
> of the compilers.
That's up for debate, I would say :-) (sorry).
In all the binaries I looked at, the const qualifier is added only on
the type of the this-pointer parameter carried by the concrete inlined
instance of a member function. The abstract instance that actually
reflects what the user code is doesn't have that const qualifier.
In other words, the const qualifier is artificially added by the
compiler to the inlined concrete instance of the function.
Ultimately, that const qualifier has no material impact on the ABI, as
far as I can tell.
So rather than having to spend energy to try and figure out where it
comes from and reason where there, I thought that stripping it off
might do as well.
I hope this explains my reasoning a bit better.
Thanks for following up.
[...]
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ir, dwarf-reader: Peel const-qualifier from const this pointers
2024-03-06 15:15 Dodji Seketeli
@ 2024-03-06 15:43 ` Ben Woodard
0 siblings, 0 replies; 4+ messages in thread
From: Ben Woodard @ 2024-03-06 15:43 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail
[-- Attachment #1: Type: text/plain, Size: 4302 bytes --]
On Wed, Mar 6, 2024 at 7:15 AM Dodji Seketeli <dodji@redhat.com> wrote:
> Hello Ben,
>
> Ben Woodard <woodard@redhat.com> a écrit:
>
> > I believe that I have a bug open on that one.
>
> Yes, there are a series of bugs of yours related to that, I think. I
> need to re-test them to see where we stand today. A number of other
> things have been fixed since then, so I might need to take another
> view at the whole thing.
>
> > Are you addressing it or did you independently find the problem?
>
> I was looking at something else (abidb), but I had your issues in mind
> too when I stumbled upon this.
>
> > This is also something that causes false positive ABI problems between
> > different toolchains.
>
> Exactly. This patch reduces the distance between the output of g++ and
> clang, as shown by the change that the patch has on the test result
> tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt.
>
>
> > The question for me though, is this the “right” fix? I remember
> > reasoning through the DWARF and wondering if a “this” pointer really
> > should be const indicating that it could be a bug in the compilers
> > when it is not. This makes me wonder if peeling the const pointer is
> > the right solution or if it is just expedient given the current state
> > of the compilers.
>
> That's up for debate, I would say :-) (sorry).
Yeah I know. I couldn’t puzzle through that one. I think that we should
probably first bring it up to the compiler guys and get their take on it. I
kind of think that it is a compiler bug and if it is not, and there is a
good reason behind it. Then we might learn something that we should take to
dwarf-discuss. Based on you explanation which was clearer than my
understanding, I think that the compiler guys are a the right first place
to discuss this rather than dwarf-discuss.
>
>
> In all the binaries I looked at, the const qualifier is added only on
> the type of the this-pointer parameter carried by the concrete inlined
> instance of a member function. The abstract instance that actually
> reflects what the user code is doesn't have that const qualifier.
>
This is the part that kind of gives me misgivings. I fear that when we get
to the point of properly handling inline functions, this might come back to
haunt us.
Here is a potential scenario which kind of concerns me and I agree upfront
that it may be bordering on function semantics rather than ABI.
Let’s say that a programmer changes the optimization flags and a function
moves from being inlined within an app due to some optimization opportunity
but also available in the library to being in an object file. Is the app
that contains the compiled in version of the function still ABI compatible
with the new version of library which has the function in a library. One of
the things that we hoped to detect here but never got to before we ran out
of time on the project was ensuring that different optimization flags
didn’t impact ABI in undetectable ways. I will admit that I never got a
handle on that part of the project.
One of the things that we worried about in the ABI project here is embedded
concrete instances of functions which are inlined ending up being
semantically different than newer versions in a later version of the
library. The best that I could offer was we could check function prototypes
but there was no way to detect semantic differences for inline functions.
> In other words, the const qualifier is artificially added by the
> compiler to the inlined concrete instance of the function.
>
> Ultimately, that const qualifier has no material impact on the ABI, as
> far as I can tell.
>
> So rather than having to spend energy to try and figure out where it
> comes from and reason where there, I thought that stripping it off
> might do as well.
I kind of agree with some not well defined misgivings but I think that we
should discuss with the compiler people it and pay attention to see if it
has unforeseen implications when we get to analyzing inline functions.
-ben
>
>
> I hope this explains my reasoning a bit better.
>
> Thanks for following up.
>
> [...]
>
> Cheers,
>
> --
> Dodji
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-03-06 15:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-06 13:09 [PATCH] ir,dwarf-reader: Peel const-qualifier from const this pointers Dodji Seketeli
2024-03-06 14:51 ` [PATCH] ir, dwarf-reader: " Ben Woodard
2024-03-06 15:15 Dodji Seketeli
2024-03-06 15:43 ` Ben Woodard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).