[Bug libstdc++/111050] New: [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[Bug libstdc++/111050] New: [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

            Bug ID: 111050
           Summary: [11/12/13/14 Regression] ABI break in
                    _Hash_node_value_base since GCC 11
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: ABI
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rs2740 at gmail dot com
  Target Milestone: ---

https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=1b6f0476837205932613ddb2b3429a55c26c409d
changed _Hash_node_value_base to no longer derive from _Hash_node_base, which
means that its member functions expect _M_storage to be at a different offset.
So explosions result if an out-of-line definition is emitted for any of the
member functions (say, in a non-optimized build) and the resulting object file
is then linked with code built using older version of GCC/libstdc++.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-08-17
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
   Target Milestone|---                         |11.5

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[fdumont at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

François Dumont <fdumont at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fdumont at gcc dot gnu.org

--- Comment #1 from François Dumont <fdumont at gcc dot gnu.org> ---
A 3 years old abi regression seems difficult to fix now. To do so we would need
to break abi again.

It seems to be a limited issue as you need a non-optimized build. The only
impacted member is the _M_next() which is a simple static_cast, I'm very
surprised that it's not always inlined even if non-optimized.

Apart perhaps documenting it I cannot think of anything to do.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #2 from TC <rs2740 at gmail dot com> ---
The impacted members we observed are `_Hash_node_value_base::_M_valptr` and
`_Hash_node_value_base::_M_v`. I think the layout of `_Hash_node` didn't
change.

And I'm not seeing why fixing this will require breaking ABI again. For
example, if the affected functions are marked always_inline (or renamed, or
have their mangling otherwise changed), I would expect the resulting code to be
linkable with either the GCC10 version or the current version of the code,
unless I'm missing something?

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[fdumont at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #3 from François Dumont <fdumont at gcc dot gnu.org> ---
For sure _Hash_node layout didn't change, that's why I couldn't think of any
abi issue here.

I see that you already had the solution ! It was some kind of test then, I
failed :-).

Did you try it ? If you already have the use case on your side to reproduce the
explosion it should be rather easy to confirm that adding:

[[__gnu__::__always_inline__]]

on _M_next member fixes the issue.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[fdumont at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

François Dumont <fdumont at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |fdumont at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #4 from François Dumont <fdumont at gcc dot gnu.org> ---
Considering that explosion is taking place in your code built with a gcc
post-11 while calling _M_next() on a _Hashtable instance node coming from a .so
built with a gcc pre-11 version I think your solution should work.

I'll send a patch proposal to mailing list.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #5 from TC <rs2740 at gmail dot com> ---
Minimal example:

$ cat lib1.cpp
#include <unordered_set>
#include <string>

static std::unordered_set<std::string> set;

void del(const std::string& s) {
    set.erase(s);
}

$ cat lib2.cpp
#include <unordered_set>
#include <string>

static std::unordered_set<std::string> set;

void add(const std::string& s) {
    set.emplace(s);
}

const std::string& get(const std::string& s) {
    return *set.find(s);
}

$ cat main.cpp

#include <string>

void add(const std::string&);
void del(const std::string&);
const std::string& get(const std::string&);

int main() {
    add("foo");
    del("bar");
    (void) get("foo").size();
}

$ g++-10 -std=c++17 lib1.cpp -c -o lib1.o
$ g++-13 -std=c++17 lib2.cpp -c -o lib2.o
$ g++-13 -std=c++17 main.cpp lib1.o lib2.o -o test
$ ./test
Segmentation fault (core dumped)

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #6 from TC <rs2740 at gmail dot com> ---
The crash is gone if lib2.o is compiled with bits/hashtable_policy.h patched
like so:

--- a/path/to/gcc-13/include/c++/13.2.0/bits/hashtable_policy.h
+++ b/hashtable_policy.h
@@ -327,18 +327,22 @@ namespace __detail

       __gnu_cxx::__aligned_buffer<_Value> _M_storage;

+      [[__gnu__::__always_inline__]]
       _Value*
       _M_valptr() noexcept
       { return _M_storage._M_ptr(); }

+      [[__gnu__::__always_inline__]]
       const _Value*
       _M_valptr() const noexcept
       { return _M_storage._M_ptr(); }

+      [[__gnu__::__always_inline__]]
       _Value&
       _M_v() noexcept
       { return *_M_valptr(); }

+      [[__gnu__::__always_inline__]]
       const _Value&
       _M_v() const noexcept
       { return *_M_valptr(); }

I'm following up with my reporter to see if this also fixes the problem with
their actual code.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #7 from TC <rs2740 at gmail dot com> ---
Confirmed with my reporter that this fixes their actual code too.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to François Dumont from comment #1)
> It seems to be a limited issue as you need a non-optimized build.

That's not a safe assumption. Inlining decisions can change across builds and
across architectures, and it's not safe to assume the affected functions will
always be inlined, e.g. in the presence of explicit instantiation definitions.

> The only
> impacted member is the _M_next() which is a simple static_cast, I'm very
> surprised that it's not always inlined even if non-optimized.

No functions are inlined for non-optimized builds, unless forced with
always_inline.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[frs.dumont at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #9 from frs.dumont at gmail dot com ---
To be honest before that report I thought that preserving abi was just a 
matter of preserving memory layout of types. I had no idea that member 
methods mattered !

Lesson learned.

On 11/09/2023 13:52, redi at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050
>
> --- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> (In reply to François Dumont from comment #1)
>> It seems to be a limited issue as you need a non-optimized build.
> That's not a safe assumption. Inlining decisions can change across builds and
> across architectures, and it's not safe to assume the affected functions will
> always be inlined, e.g. in the presence of explicit instantiation definitions.
>
>> The only
>> impacted member is the _M_next() which is a simple static_cast, I'm very
>> surprised that it's not always inlined even if non-optimized.
> No functions are inlined for non-optimized builds, unless forced with
> always_inline.
>

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[john at drouhard dot dev]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

John Drouhard <john at drouhard dot dev> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |john at drouhard dot dev

--- Comment #10 from John Drouhard <john at drouhard dot dev> ---
(In reply to frs.dumont from comment #9)
> To be honest before that report I thought that preserving abi was just a 
> matter of preserving memory layout of types. I had no idea that member 
> methods mattered !

(I was the original reporter of this to TC)

I think the specific issue here is that the member function `_M_valptr()`
returns the address of the storage data member, and that _function_ is used in
a construct call elsewhere to point to the address where a new object should be
placed. It returns the address based on the offset from the beginning of the
object which changed when the base class (which had its own data members) was
removed.

So, if the function isn't inlined, the symbol that's actually loaded by the
dynamic linker during runtime will return a potentially bogus address for that
data member if the definition of that function came from a library compiled
with the other version.

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Right. The _M_valptr() function compiles to something like:

return (_Value*)((char*)this + offsetof(_Hash_node_value_base, _M_storage);

In GCC 10, the offsetof expression was non-zero, specifically it was
sizeof(_Hash_node_base). So the _M_valptr() function emitted by GCC 10 expected
to access the storage member at a non-zero offset to the 'this' pointer
pointing to the _Hash_node_value_base subobject.

In GCC 11+ the offsetof expression is zero, so the definition of _M_valptr()
emitted by GCC 11 does something different. That's an ABI break.

If the linker chooses the GCC 10 definition of the function, but the function
gets called with a 'this' pointer from GCC 11 code, it will apply the non-zero
offset when it shouldn't (and vice versa).

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Francois Dumont <fdumont@gcc.gnu.org>:

https://gcc.gnu.org/g:2c1e3544a94c5d7354fad031e1f9731c3ce3af25

commit r14-4313-g2c1e3544a94c5d7354fad031e1f9731c3ce3af25
Author: Tim Song <t.canens.cpp@gmail.com>
Date:   Wed Sep 6 19:31:55 2023 +0200

    libstdc++: Force _Hash_node_value_base methods inline to fix abi (PR111050)

   
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=1b6f0476837205932613ddb2b3429a55c26c409d
    changed _Hash_node_value_base to no longer derive from _Hash_node_base,
which means
    that its member functions expect _M_storage to be at a different offset. So
explosions
    result if an out-of-line definition is emitted for any of the member
functions (say,
    in a non-optimized build) and the resulting object file is then linked with
code built
    using older version of GCC/libstdc++.

    libstdc++-v3/ChangeLog:

            PR libstdc++/111050
            * include/bits/hashtable_policy.h
            (_Hash_node_value_base<>::_M_valptr(),
_Hash_node_value_base<>::_M_v())
            Add [[__gnu__::__always_inline__]].

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Francois Dumont
<fdumont@gcc.gnu.org>:

https://gcc.gnu.org/g:50ace1f8a784d08a72edb8cb4044101fdabcc072

commit r11-11028-g50ace1f8a784d08a72edb8cb4044101fdabcc072
Author: Tim Song <t.canens.cpp@gmail.com>
Date:   Wed Sep 6 19:31:55 2023 +0200

    libstdc++: Force _Hash_node_value_base methods inline to fix abi (PR111050)

   
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=1b6f0476837205932613ddb2b3429a55c26c409d
    changed _Hash_node_value_base to no longer derive from _Hash_node_base,
which means
    that its member functions expect _M_storage to be at a different offset. So
explosions
    result if an out-of-line definition is emitted for any of the member
functions (say,
    in a non-optimized build) and the resulting object file is then linked with
code built
    using older version of GCC/libstdc++.

    libstdc++-v3/ChangeLog:

            PR libstdc++/111050
            * include/bits/hashtable_policy.h
            (_Hash_node_value_base<>::_M_valptr(),
_Hash_node_value_base<>::_M_v())
            Add [[__gnu__::__always_inline__]].

    (cherry picked from commit 2c1e3544a94c5d7354fad031e1f9731c3ce3af25)

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Francois Dumont
<fdumont@gcc.gnu.org>:

https://gcc.gnu.org/g:1be57348229666c54954f1e5937cae00e113f7f1

commit r12-9903-g1be57348229666c54954f1e5937cae00e113f7f1
Author: Tim Song <t.canens.cpp@gmail.com>
Date:   Wed Sep 6 19:31:55 2023 +0200

    libstdc++: Force _Hash_node_value_base methods inline to fix abi (PR111050)

   
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=1b6f0476837205932613ddb2b3429a55c26c409d
    changed _Hash_node_value_base to no longer derive from _Hash_node_base,
which means
    that its member functions expect _M_storage to be at a different offset. So
explosions
    result if an out-of-line definition is emitted for any of the member
functions (say,
    in a non-optimized build) and the resulting object file is then linked with
code built
    using older version of GCC/libstdc++.

    libstdc++-v3/ChangeLog:

            PR libstdc++/111050
            * include/bits/hashtable_policy.h
            (_Hash_node_value_base<>::_M_valptr(),
_Hash_node_value_base<>::_M_v())
            Add [[__gnu__::__always_inline__]].

    (cherry picked from commit 2c1e3544a94c5d7354fad031e1f9731c3ce3af25)

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Francois Dumont
<fdumont@gcc.gnu.org>:

https://gcc.gnu.org/g:e6d26b141bf03a0348b51e4778c79d44dc760eed

commit r13-7931-ge6d26b141bf03a0348b51e4778c79d44dc760eed
Author: Tim Song <t.canens.cpp@gmail.com>
Date:   Wed Sep 6 19:31:55 2023 +0200

    libstdc++: Force _Hash_node_value_base methods inline to fix abi (PR111050)

   
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=1b6f0476837205932613ddb2b3429a55c26c409d
    changed _Hash_node_value_base to no longer derive from _Hash_node_base,
which means
    that its member functions expect _M_storage to be at a different offset. So
explosions
    result if an out-of-line definition is emitted for any of the member
functions (say,
    in a non-optimized build) and the resulting object file is then linked with
code built
    using older version of GCC/libstdc++.

    libstdc++-v3/ChangeLog:

            PR libstdc++/111050
            * include/bits/hashtable_policy.h
            (_Hash_node_value_base<>::_M_valptr(),
_Hash_node_value_base<>::_M_v())
            Add [[__gnu__::__always_inline__]].

    (cherry picked from commit 2c1e3544a94c5d7354fad031e1f9731c3ce3af25)

[Bug libstdc++/111050] [11/12/13/14 Regression] ABI break in _Hash_node_value_base since GCC 11

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111050

Jonathan Wakely <redi at gcc dot gnu.org> changed:

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

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 14.0, 13.3, 12.4 and 11.5. Thank you both.