[Bug libgcc/113403] New: __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[Bug libgcc/113403] New: __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[fw at gcc dot gnu.org]

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

            Bug ID: 113403
           Summary: __builtin_nested_func_ptr_created,
                    __builtin_nested_func_ptr should be dynamically linked
                    by default
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libgcc
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fw at gcc dot gnu.org
  Target Milestone: ---

There are hidden definitions of __builtin_nested_func_ptr_created,
__builtin_nested_func_ptr in libgcc.a. As a result of the default link order,
the exported global definitions of these functions in libgcc_s.so.1 are never
used.

This is unfortunate because it means that every DSO that creates a trampoline
gets its own trampoline cache (one TLS variable and two data pages for
trampolines), increasing the overhead of this feature significantly.

[Bug libgcc/113403] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[iains at gcc dot gnu.org]

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

--- Comment #1 from Iain Sandoe <iains at gcc dot gnu.org> ---

this seems a somewhat similar dilemma to the emulated TLS case.

Perhaps:

provide a weak definition of these in a CRT (which is used for -static-libgcc)
and weaken the defs in libgcc_s so that mixed scenarios work?

[Bug libgcc/113403] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[fw at gcc dot gnu.org]

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

--- Comment #2 from Florian Weimer <fw at gcc dot gnu.org> ---
Weak symbols have no meaning for ELF DSOs, so this wouldn't work for libgcc_s
on ELF targets. Instead we automatically link against dynamic libgcc_s if
certain symbols not in libgcc.a are referenced. Sources built with
-ftrampoline-impl=heap -fexceptions can already trigger such a dependency for
_Unwind_Resume because it's used in the cleanup handler, for example.

[Bug libgcc/113403] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[iains at gcc dot gnu.org]

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

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #2)
> Weak symbols have no meaning for ELF DSOs, so this wouldn't work for
> libgcc_s on ELF targets. Instead we automatically link against dynamic
> libgcc_s if certain symbols not in libgcc.a are referenced. Sources built
> with -ftrampoline-impl=heap -fexceptions can already trigger such a
> dependency for _Unwind_Resume because it's used in the cleanup handler, for
> example.

So if we remove these symbols from libgcc.a and then the user builds with
-static-libgcc - that would just be an error (presumably at runtime if ld
allows undefined symbols).  I'm not clear about what the scope of "we
automatically link against dynamic libgcc_s" is in that case.

Assuming that omitting the symbols from libgcc.a means either linking against
libgcc_s or providing them from some other source when -static-libgcc is given:

 - we could use a CRT for that.
 - I suppose we could also add these symbols to libgcc_eh for that case - but
that proved to be a bad choice for emuTLS, so it would also need to be
configured per platform.

On platforms like Mach-O, the strategy I outlined will work OK (this is
configurable per target, so should be OK).

[Bug libgcc/113403] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #2)
> Weak symbols have no meaning for ELF DSOs, so this wouldn't work for
> libgcc_s on ELF targets. Instead we automatically link against dynamic
> libgcc_s if certain symbols not in libgcc.a are referenced. Sources built
> with -ftrampoline-impl=heap -fexceptions can already trigger such a
> dependency for _Unwind_Resume because it's used in the cleanup handler, for
> example.

What we could do is make -shared-libgcc the default when using
-ftrampoline-impl=heap (similarly how linking with g++ instead of gcc makes it
the default), obviously unless use explicitly asks for -static-libgcc or
-shared-libgcc.

[Bug libgcc/113403] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[iains at gcc dot gnu.org]

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

--- Comment #5 from Iain Sandoe <iains at gcc dot gnu.org> ---

Sorry, my intended design was not stated very clearly.

1. IIUC, the objective is to have only one instance of these symbols in the
dynamically-linked program.

2. One way to ensure that is to make it a requirement that all DSOs that need
these must link with libgcc_s.

 - However, that is not always what the user wants to do and might link a DSO
with -static-libgcc
 - it makes DSOs built with GCC more difficult to use with other compilers.

3. An alternate solution is to have COMDAT/Weak Definitions (I'm not sure if
I've got exactly the right terminology for ELF; Weak Definition is the right
one for Mach-o).

 - then the dynamic linker picks only one in the loaded program, so the
duplication is avoided
 - this also works when DSOs are used with a foreign compiler without libgcc_s

---

For my ports the second is the most practical, since my downsteams use GCC and
clang together, but I have no strong opinion on what should be done in general.

[Bug libgcc/113403] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

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

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> ---
This seems somewhat similar to the C++ issue that led to introducing
STB_GNU_UNIQUE; a DSO keeping its own copy of something that we want to be
shared.  In the C++ case we were dealing with vague linkage variables that we
have no control over, and decided that the first copy that gets loaded is used
by all later DSOs, and so it can never be unloaded.

Here, we have more control over the definition, and could say that everyone
should get it from libgcc_s, as Jakub proposes.  Or we could force it to be
defined in the executable, so all DSOs will use the copy there.

That latter might be another way to handle the STB_GNU_UNIQUE situation: if an
executable links against a shared library that defines something
STB_GNU_UNIQUE, copy the definition from the library into the executable?  That
would avoid the dlclose problem.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
           Keywords|                            |ABI
            Summary|__builtin_nested_func_ptr_c |[14 Regression]
                   |reated,                     |__builtin_nested_func_ptr_c
                   |__builtin_nested_func_ptr   |reated,
                   |should be dynamically       |__builtin_nested_func_ptr
                   |linked by default           |should be dynamically
                   |                            |linked by default
   Target Milestone|---                         |14.0

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'm marking this as regression since it affects the ABI.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[fw at gcc dot gnu.org]

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

--- Comment #8 from Florian Weimer <fw at gcc dot gnu.org> ---
In the current implementation, as far as I understand it, avoiding multiple
objects is just an optimization, not a correctness issue. STB_GNU_UNIQUE is for
correctness (although I don't think we'd implement it this way today).

The deallocation API does not explicitly reference the closure, which is why
stackful coroutines/fibers will have to switch the allocator state as part of
the context switch. Once we start doing this, it simplifies matters greatly if
there is just one such state to switch, and not an arbitrary count.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Note, normally libgcc.a symbols are made .hidden (and that is the case of even
these 2 functions).  So, when not using -shared-libgcc (implicitly or
explicitly), every shared library or binary uses its own copy of the registry,
rather than a shared one.
So, if we wanted to make the symbols STB_GNU_UNIQUE, we'd also need to stop
making them .hidden.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[iains at gcc dot gnu.org]

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

--- Comment #10 from Iain Sandoe <iains at gcc dot gnu.org> ---
it is an optimisation, yes - but as Richi points out, if we change this it will
affect ABI - so it is ideal to do this before the first release that includes
it?


- IIUC Jakub's suggestion:
 - remove the functions from libgcc.a
 - add a spec to gcc/gcc.cc like 
  %{!static:%(!static-libgcc:%(ftrampoline-impl=heap: -shared-libgcc }}}

So the symbols would be public and in libgcc_s only (unless a target chooses to
do something different).

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Iain Sandoe from comment #10)
> it is an optimisation, yes - but as Richi points out, if we change this it
> will affect ABI - so it is ideal to do this before the first release that
> includes it?
> 
> 
> - IIUC Jakub's suggestion:
>  - remove the functions from libgcc.a

Not necessarily, just maybe.
We do that thing with libgcc_eh.a (unwinder is in libgcc_eh.a and libgcc_s.so.1
and not in libgcc.a).
Or put it in libgcc_eh.a and libgcc_s.so.1?
It is not unwinder, but shares the central registry property with it.

>  - add a spec to gcc/gcc.cc like 
>   %{!static:%(!static-libgcc:%(ftrampoline-impl=heap: -shared-libgcc }}}

Either that, or do something like cp/g++spec.cc does with shared_libgcc.
Whatever works.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[fw at gcc dot gnu.org]

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

--- Comment #12 from Florian Weimer <fw at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #11)
> Or put it in libgcc_eh.a and libgcc_s.so.1?

Yes, that's what I came up with as well (conceptually, not a patch, and I only
have a background in ELF), but Iain already dismissed it in comment 3.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[iains at gcc dot gnu.org]

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

--- Comment #13 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #12)
> (In reply to Jakub Jelinek from comment #11)
> > Or put it in libgcc_eh.a and libgcc_s.so.1?
> 
> Yes, that's what I came up with as well (conceptually, not a patch, and I
> only have a background in ELF), but Iain already dismissed it in comment 3.

I would not do that for Darwin, experience with emuTLS gave us headaches in
doing that, especially since our users are often building projects using both
GCC and clang.

However, this is an area where individual targets can have different solutions
- so my comment should not be seen to apply to ELF targets if that seems the
best remedy there.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[iains at gcc dot gnu.org]

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

--- Comment #14 from Iain Sandoe <iains at gcc dot gnu.org> ---
Created attachment 57182
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57182&action=edit
patch under test

this is what I'm testing
 - these functions are removed from libgcc.a and added to libgcc_eh.a
 - since most versions of Darwin cannot use libgcc_eh, it arranges to make a
CRT containing them that can be linked in cases that require it.
 - for Darwin, we also make the definitions weak and exported where used in
DSOs, this means that a dynamically linked program should only end up with one
copy of the cache even if the shared libgcc is not in use.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:7b3b3788c579856abcfdc6eed589c64dc7e88cdb

commit r14-8520-g7b3b3788c579856abcfdc6eed589c64dc7e88cdb
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Fri Jan 19 15:57:04 2024 +0000

    libgcc: Make heap trampoline support dynamic [PR113403].

    This removes the heap trampoline support functions from libgcc.a and
    adds them to libgcc_eh.a.  They are also present in libgcc_s.

            PR libgcc/113403

    libgcc/ChangeLog:

            * config/aarch64/t-heap-trampoline: Move the heap trampoline
            support functions from libgcc.a to libgcc_eh.a.
            * config/i386/t-heap-trampoline: Likewise.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[cvs-commit at gcc dot gnu.org]

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

--- Comment #16 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:506e74f53a5e4f607284d3c41da17cdd3eca4fb8

commit r14-8521-g506e74f53a5e4f607284d3c41da17cdd3eca4fb8
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Sun Jan 28 13:31:56 2024 +0000

    libgcc: Make heap trampoline support dynamic [PR113403].

    In order to handle system security constraints during GCC build
    and test and that most platform versions cannot link to libgcc_eh
    since the unwinder there is incompatible with the system one.

    1. We make the support functions weak definitions.
    2. We include them as a CRT for platform conditions that do not
       allow libgcc_eh.
    3. We ensure that the weak symbols are exported from DSOs (which
       includes exes on Darwin) so that the dynamic linker will
       pick one instance (which avoids duplication of trampoline
       caches).

            PR libgcc/113403

    gcc/ChangeLog:

            * config/darwin.h (DARWIN_SHARED_WEAK_ADDS, DARWIN_WEAK_CRTS): New.
            (REAL_LIBGCC_SPEC): Move weak CRT handling to separate spec.
            * config/i386/darwin.h (DARWIN_HEAP_T_LIB): New.
            * config/i386/darwin32-biarch.h (DARWIN_HEAP_T_LIB): New.
            * config/i386/darwin64-biarch.h (DARWIN_HEAP_T_LIB): New.
            * config/rs6000/darwin.h (DARWIN_HEAP_T_LIB): New.

    libgcc/ChangeLog:

            * config.host: Build libheap_t.a for i686/x86_64 Darwin.
            * config/aarch64/heap-trampoline.c (HEAP_T_ATTR): New.
            (allocate_tramp_ctrl): Allow a target to build this as a weak def.
            (__gcc_nested_func_ptr_created): Likewise.
            * config/i386/heap-trampoline.c (HEAP_T_ATTR): New.
            (allocate_tramp_ctrl): Allow a target to build this as a weak def.
            (__gcc_nested_func_ptr_created): Likewise.
            * config/t-darwin: Build libheap_t.a (a CRT with heap trampoline
            support).

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think this is fixed now.  Please reopen if you disagree.

[Bug libgcc/113403] [14 Regression] __builtin_nested_func_ptr_created, __builtin_nested_func_ptr should be dynamically linked by default

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:03519175e21f4c2940aeb446cd2b81fdf995cad5

commit r14-8711-g03519175e21f4c2940aeb446cd2b81fdf995cad5
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Feb 1 21:05:58 2024 +0100

    libgcc: Fix up i386/t-heap-trampoline [PR113403]

    I'm seeing
    ../../../libgcc/shared-object.mk:14: warning: overriding recipe for target
'heap-trampoline.o'
    ../../../libgcc/shared-object.mk:14: warning: ignoring old recipe for
target 'heap-trampoline.o'
    ../../../libgcc/shared-object.mk:17: warning: overriding recipe for target
'heap-trampoline_s.o'
    ../../../libgcc/shared-object.mk:17: warning: ignoring old recipe for
target 'heap-trampoline_s.o'

    This patch fixes that.

    2024-02-01  Jakub Jelinek  <jakub@redhat.com>

            PR libgcc/113403
            * config/i386/t-heap-trampoline: Add to LIB2ADDEHSHARED
            i386/heap-trampoline.c rather than aarch64/heap-trampoline.c.