Hi Prathamesh!

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?

Does libatomic even need a switch corresponding to '-static-libgcc'?

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?

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.

..., 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?


> 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?


> --- 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?


> --- 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'?

Also, if enabled by default, don't we usually describe the negative form
('-fno-link-libatomic') in the manual?


Grüße
 Thomas

Reply via email to