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
