Sam James <s...@gentoo.org> writes:
> Prathamesh Kulkarni <prathame...@nvidia.com> writes:
>
>>> -----Original Message-----
>>> From: Prathamesh Kulkarni <prathame...@nvidia.com>
>>> Sent: 10 January 2025 09:48
>>> To: Thomas Schwinge <tschwi...@baylibre.com>
>>> Cc: Tobias Burnus <tbur...@baylibre.com>; Joseph Myers
>>> <josmy...@redhat.com>; Xi Ruoyao <xry...@xry111.site>; Matthew
>>> Malcomson <mmalcom...@nvidia.com>; gcc-patches@gcc.gnu.org; Tom de
>>> Vries <tdevr...@suse.de>
>>> Subject: RE: [RFC] PR81358: Enable automatic linking of libatomic
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> > -----Original Message-----
>>> > From: Thomas Schwinge <tschwi...@baylibre.com>
>>> > Sent: 07 January 2025 17:45
>>> > To: Prathamesh Kulkarni <prathame...@nvidia.com>
>>> > Cc: Tobias Burnus <tbur...@baylibre.com>; Joseph Myers
>>> > <josmy...@redhat.com>; Xi Ruoyao <xry...@xry111.site>; Matthew
>>> > Malcomson <mmalcom...@nvidia.com>; gcc-patches@gcc.gnu.org; Tom de
>>> > Vries <tdevr...@suse.de>
>>> > Subject: RE: [RFC] PR81358: Enable automatic linking of libatomic
>>> >
>>> > External email: Use caution opening links or attachments
>>> >
>>> >
>>> > Hi Prathamesh!
>>> Hi Thomas, thanks for the review!
>>> >
>>> > Thanks for working on this!
>>> >
>>> >
>>> > Per my understanding, this patch won't automagically resolve the
>>> need
>>> > to
>>> > (occasionally) having to specify '-foffload-options=nvptx-none=-
>>> > latomic'
>>> > for nvptx offloading: it doesn't use 'LINK_LIBATOMIC_SPEC',
>>> currently
>>> > only used via 'GNU_USER_TARGET_LINK_GCC_C_SEQUENCE_SPEC' from
>>> > 'gcc/config/gnu-user.h' (general issue, affecting a lot of
>>> > configurations, to be addressed as necessary):
>>> >
>>> > > --- a/gcc/config/gnu-user.h
>>> > > +++ b/gcc/config/gnu-user.h
>>> >
>>> > > #define GNU_USER_TARGET_LINK_GCC_C_SEQUENCE_SPEC \
>>> > > - "%{static|static-pie:--start-group} %G %{!nolibc:%L} \
>>> > > + "%{static|static-pie:--start-group} %G %{!nolibc:"
>>> > > + LINK_LIBATOMIC_SPEC "%L} \
>>> > > %{static|static-pie:--end-group}%{!static:%{!static-pie:%G}}"
>>> >
>>> > > --- a/gcc/gcc.cc
>>> > > +++ b/gcc/gcc.cc
>>> >
>>> > > /* Here is the spec for running the linker, after compiling all
>>> > > files. */
>>> > >
>>> > > +#if defined(TARGET_PROVIDES_LIBATOMIC) &&
>>> defined(USE_LD_AS_NEEDED)
>>> > > +#define LINK_LIBATOMIC_SPEC "%{!fno-link-libatomic:"
>>> > LD_AS_NEEDED_OPTION \
>>> > > + " -latomic " LD_NO_AS_NEEDED_OPTION "} "
>>> > > +#else
>>> > > +#define LINK_LIBATOMIC_SPEC ""
>>> > > +#endif
>>> > > +
>>> > > /* This is overridable by the target in case they need to specify
>>> > the
>>> > > -lgcc and -lc order specially, yet not require them to
>>> override
>>> > all
>>> > > of LINK_COMMAND_SPEC. */
>>> >
>>> > ..., and the nvptx linker doesn't support '--as-needed'.
>>> >
>>> > I'll think about it; indeed it'd be good to get that resolved, too.
>>> >
>>> >
>>> > On 2024-12-20T15:37:42+0000, Prathamesh Kulkarni
>>> > <prathame...@nvidia.com> wrote:
>>> > > [...] copying libatomic.a over to $(gcc_objdir)$(MULTISUBDIR)/,
>>> and
>>> > > can confirm that 64-bit libatomic.a is copied to $build/gcc/ and
>>> 32-
>>> > bit libatomic.a is copied to $build/gcc/32/.
>>> >
>>> > So this:
>>> >
>>> > > --- a/libatomic/Makefile.am
>>> > > +++ b/libatomic/Makefile.am
>>> >
>>> > > @@ -162,6 +162,11 @@ libatomic_convenience_la_LIBADD =
>>> > > $(libatomic_la_LIBADD) # when it is reloaded during the build of
>>> > all-multi.
>>> > > all-multi: $(libatomic_la_LIBADD)
>>> > >
>>> > > +gcc_objdir = $(MULTIBUILDTOP)../../$(host_subdir)/gcc
>>> > > +all: all-multi libatomic.la libatomic_convenience.la
>>> > > + $(INSTALL_DATA) .libs/libatomic.a
>>> $(gcc_objdir)$(MULTISUBDIR)/
>>> > > + chmod 644 $(gcc_objdir)$(MULTISUBDIR)/libatomic.a
>>> >
>>> > ... is conceptually modelled after libgcc, where the libraries get
>>> > copied into 'gcc/'? However, here we only copy the static
>>> > 'libatomic.a'; libgcc does a 'make install-leaf', see
>>> > 'libgcc/Makefile.in':
>>> >
>>> > all: all-multi
>>> > # Now that we have built all the objects, we need to copy
>>> > # them back to the GCC directory. Too many things (other
>>> > # in-tree libraries, and DejaGNU) know about the layout
>>> > # of the build tree, for now.
>>> > $(MAKE) install-leaf DESTDIR=$(gcc_objdir) \
>>> > slibdir= libsubdir= MULTIOSDIR=$(MULTIDIR)
>>> >
>>> > ..., which also installs dynamic libraries. Is that difference
>>> > intentional and/or possibly important?
>>> Well, I wasn't sure what extension to use for shared libraries, and
>>> initially avoided copying them.
>>> libgcc seems to use $(SHLIB_EXT) to specify extension name for shared
>>> libraries, which can be overridden by targets.
>>>
>>> Matthew pointed out to me that using libtool --mode=install works for
>>> copying both static and shared libraries (with the numbered version
>>> libatomic.so.1.2.0), so in the attached patch, I changed Makefile.am
>>> rule to following:
>>> gcc_objdir = `pwd`/$(MULTIBUILDTOP)../../gcc/
>>> all: all-multi libatomic.la libatomic_convenience.la
>>> $(LIBTOOL) --mode=install $(INSTALL_DATA) libatomic.la
>>> $(gcc_objdir)$(MULTISUBDIR)/
>>>
>>> Which seems to install libatomic.a, libatomic.so and the numbered
>>> version in $build/gcc/ and in $build/gcc/32/ (and $build/gcc/mgomp/
>>> for nvptx build).
>>> Does it look OK ?
>>> >
>>> > Does libatomic even need a switch corresponding to '-static-libgcc'?
>>> I am not sure, hoping for Joseph to chime in.
>>> >
>>> > Given that libatomic libraries get copied into 'gcc/', will we be
>>> able
>>> > (later, incrementally) to remove some setup code from the test
>>> suites'
>>> > '*.exp' files, to locate build-tree libatomic?
>>> I'd guess so.
>>> >
>>> > Also, given the presumed similarity to how libgcc is handled (with,
>>> of
>>> > course, the difference that libatomic isn't built for all
>>> > configurations), should we maybe in the build system place the new
>>> > libatomic handling next to the existing libgcc handling?
>>> > Specifically, instead of:
>>> >
>>> > > --- a/Makefile.def
>>> > > +++ b/Makefile.def
>>> >
>>> > > +dependencies = { module=configure-target-libbacktrace;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libgloss; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-newlib;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libgomp; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libitm;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libstdc++v3; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libsanitizer;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libvtv; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libssp;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libquadmath; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libgfortran;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libffi; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libobjc;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libada; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libgm2;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libgo; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-libgrust;
>>> > > +on=all-target-libatomic; }; dependencies = {
>>> > > +module=configure-target-libphobos; on=all-target-libatomic; };
>>> > > +dependencies = { module=configure-target-zlib;
>>> > > +on=all-target-libatomic; };
>>> >
>>> > ... handle libatomic like:
>>> >
>>> > // [...] By default target modules depend
>>> > // on libgcc and newlib/libgloss.
>>> The patch adjusts Makefile.tpl to add no_atomic to
>>> lang_env_dependencies, and adding dependency on libatomic if the
>>> attribute is not set for target library, similar to others (no_gcc,
>>> no_c). This also fixes the newlib failure with offloading.
>>> Does it look OK ?
>>> >
>>> > ..., and regarding:
>>> >
>>> > > --- a/configure.ac
>>> > > +++ b/configure.ac
>>> >
>>> > > +# If we are building libatomic, bootstrap it.
>>> > > +if echo " ${target_configdirs} " | grep " libatomic " > /dev/null
>>> > > +2>&1 ; then
>>> > > + bootstrap_target_libs=${bootstrap_target_libs}target-libatomic,
>>> > > +fi
>>> >
>>> > ..., maybe place that right after:
>>> >
>>> > # Target libraries that we bootstrap.
>>> > bootstrap_target_libs=,target-libgcc,
>>> >
>>> > But I haven't spent a lot of thought on these items, so maybe that
>>> > doesn't make sense.
>>> >
>>> >
>>> > > (2) libatomic_convenience.la was not getting generated for some
>>> > reason, which resulted in build failure while building libdruntime.
>>> > > The patch adds libatomic_convenience.la as a dependency, and I can
>>> > see it now getting generated, which seems to fix the build issue
>>> with
>>> > libdruntime.
>>> >
>>> > It's not obvious to me why 'libatomic_convenience' belongs onto this
>>> > 'all' rule, given that we don't do anything with
>>> > 'libatomic_convenience'
>>> > there?
>>> Because it broke libdruntime. Quoting from my previous reply:
>>> "libatomic_convenience.la was not getting generated for some reason,
>>> which resulted in build failure while building libdruntime.
>>> The patch adds libatomic_convenience.la as a dependency, and I can see
>>> it now getting generated, which seems to fix the build issue with
>>> libdruntime."
>>> >
>>> >
>>> > > Patch passes bootstrap+test with multilib enabled for --enable-
>>> > languages=all on x86_64-linux-gnu, and for --enable-
>>> > languages=c,c++,fortran on aarch64-linux-gnu.
>>> > > Does this version look OK ?
>>> >
>>> > For nvptx target, with newlib sources sym-linked into a combined
>>> tree,
>>> > the build fails:
>>> >
>>> > [...]
>>> > make[2]: Leaving directory
>>> > '/home/thomas/tmp/source/gcc/build/queue-slim-omp/build-gcc-offload-
>>> > nvptx-none/nvptx-none/libgcc'
>>> > make[1]: Circular configure-target-libatomic <- maybe-all-
>>> target-
>>> > newlib dependency dropped.
>>> > Checking multilib configuration for libatomic...
>>> > mkdir -p -- nvptx-none/libatomic
>>> > Configuring in nvptx-none/libatomic
>>> > [...]
>>> > checking whether the C compiler works... no
>>> > configure: error: in `/home/thomas/tmp/source/gcc/build/queue-
>>> > slim-omp/build-gcc-offload-nvptx-none/nvptx-none/libatomic':
>>> > configure: error: C compiler cannot create executables
>>> > [...]
>>> >
>>> > This is, per my understanding, because libatomic is attempted to be
>>> > built before newlib, but the former depends on the latter.
>>> >
>>> > So, the "Circular [...] dependency" will need resolving, I suppose?
>>> Fixed by the above change.
>>> >
>>> >
>>> > > --- a/gcc/common.opt
>>> > > +++ b/gcc/common.opt
>>> >
>>> > > +flink-libatomic
>>> > > +Common Driver Var(flag_link_libatomic) Init(1)
>>> >
>>> > 'gcc/common.opt.urls' needs updating, I suppose?
>>> Done, thanks.
>>> >
>>> >
>>> > > --- a/gcc/doc/invoke.texi
>>> > > +++ b/gcc/doc/invoke.texi
>>> > > @@ -206,7 +206,7 @@ in the following sections.
>>> > > -fpermitted-flt-eval-methods=@var{standard}
>>> > > -fplan9-extensions -fsigned-bitfields -funsigned-bitfields
>>> > > -fsigned-char -funsigned-char -fstrict-flex-arrays[=@var{n}]
>>> > > --fsso-struct=@var{endianness}}
>>> > > +-flink-libatomic -fsso-struct=@var{endianness}}
>>> > >
>>> > > @item C++ Language Options
>>> > > @xref{C++ Dialect Options,,Options Controlling C++ Dialect}.
>>> > > @@ -2899,6 +2899,10 @@ The @option{-fstrict_flex_arrays} option
>>> > > interacts with the @option{-Wstrict-flex-arrays} option.
>>> > > @xref{Warning Options}, for more information.
>>> > >
>>> > > +@opindex flink-libatomic
>>> > > +@item -flink-libatomic
>>> > > +Enable linking of libatomic if it's supported by target. Enabled
>>> by
>>> > default.
>>> > > +
>>> > > @opindex fsso-struct
>>> > > @item -fsso-struct=@var{endianness} Set the default scalar
>>> storage
>>> > > order of structures and unions to
>>> > the
>>> >
>>> > Why place '-flink-libatomic' between '-fstrict-flex-arrays' and '-
>>> > fsso-struct'?
>>> Moved it to end.
>>> >
>>> > Also, if enabled by default, don't we usually describe the negative
>>> > form
>>> > ('-fno-link-libatomic') in the manual?
>>> Fixed, thanks.
>>>
>>> I am re-validating the patch for following configs:
>>> (a) Bootstrap+test with multilib enabled for all languages on x86_64-
>>> linux-gnu.
>>> (b) Bootstrap+test for c,c++,fortran on aarch64-linux-gnu.
>>> (c) Cross testing on x86_64->aarch64.
>>> (d) Test offloading with nvptx.
>>>
>>> Does the patch look OK if it passes testing for above configs ?
>> Hi Joseph,
>> Does the above patch in:
>> https://gcc.gnu.org/pipermail/gcc-patches/2025-January/673155.html look OK
>> to commit (altho it's stage-4 now) ?
>> It fixes all the fallouts observed so far (multilib, nvptx offloading,
>> libdruntime).
>
> So far, testing has gone well on my end (multilib issues fixed too), but
> I suspect it introduced an issue with RPATH:
> https://bugs.gentoo.org/948103.
>
> Our QA tooling reports:
> * QA Notice: The following files contain insecure RUNPATHs
> * Please file a bug about this at https://bugs.gentoo.org/
> * with the maintainer of the package.
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/libcc1.so.0.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/libstdc++.so.6.0.34n
> RPATH:
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/liblsan.so.0.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/libasan.so.8.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/libubsan.so.1.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/libtsan.so.2.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/libhwasan.so.0.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/32/libstdc++.so.6.0.34n
> RPATH:
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc/32
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/32/libasan.so.8.0.0n
> RPATH:
> /usr/lib/../lib:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc/32
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/32/libubsan.so.1.0.0n
> RPATH:
> /usr/lib/../lib:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc/32
> *
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/plugin/libcp1plugin.so.0.0.0n
> RPATH:
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
> *
>
> /var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/image/usr/lib/gcc/x86_64-pc-linux-gnu/15/plugin/libcc1plugin.so.0.0.0n
> RPATH:
>
> /usr/lib/../lib64:/var/tmp/portage/sys-devel/gcc-15.0.0_pre20250112-r2/work/build/./gcc
Prathamesh, do you have plans to look at this for 15, or should I drop
the patch from our builds for now?
thanks,
sam
