Hi Prathamesh!
On 2025-01-10T04:17:52+0000, Prathamesh Kulkarni <prathame...@nvidia.com> wrote:
>> -----Original Message-----
>> From: Thomas Schwinge <tschwi...@baylibre.com>
>> Sent: 07 January 2025 17:45
>> 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/'?
Could you please clarify that this is indeed the case, or if there's a
different reason for doing it this way ("libraries get copied into
'gcc/'"), and overall what the rationale is -- is it "just convenience"
(meaning: not having to set up search paths to the actual libatomic
build), or something else? I suggest you actually put a comment next to
this new 'libatomic/Makefile.am' rule, also cross-referencing the
corresponding libgcc thing, as applicable.
>> 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 ?
I confirm this appears to work as intended -- but I can't comment on
whether it's conceptually the right thing to do.
>> 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; };
>> > [Etc.]
>>
>> ... 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 ?
Ah, great, that looks much better in my opinion. I'm not an expert of
the top-level build system, but I confirm the combined-tree GCC plus
newlib does build fine for nvptx offloading.
>> ..., 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."
Yeah, but it's not clear to me, *why*. So I had a look.
In 'libatomic/Makefile.in' we currently have:
all: auto-config.h
$(MAKE) $(AM_MAKEFLAGS) all-recursive
..., and we're now adding:
+all: all-multi libatomic.la libatomic_convenience.la
+ $(LIBTOOL) --mode=install $(INSTALL_DATA) libatomic.la
$(gcc_objdir)$(MULTISUBDIR)/
Experiment (instead of reading 'info make', I suppose):
$ cat < Makefile
all: auto-config.h
echo old 'all' rule
all: all-multi libatomic.la libatomic_convenience.la
echo new 'all' rule
auto-config.h all-multi libatomic.la libatomic_convenience.la:
: $@
$ make
Makefile:5: warning: overriding recipe for target 'all'
Makefile:2: warning: ignoring old recipe for target 'all'
: all-multi
: libatomic.la
: libatomic_convenience.la
: auto-config.h
echo new 'all' rule
new all rule
That means, the "old 'all' rule" is no longer being run!
Indeed in the GCC build log I now see:
[...]
make[2]: Entering directory
'[...]/build-gcc-offload-nvptx-none/nvptx-none/libatomic'
Makefile:906: warning: overriding recipe for target 'all'
Makefile:469: warning: ignoring old recipe for target 'all'
[...]
..., which means that libatomic's original:
all: auto-config.h
$(MAKE) $(AM_MAKEFLAGS) all-recursive
... is no longer being run, which -- supposedly -- is "recovered" (in one
way or another) by adding the 'libatomic_convenience.la' dependency.
Instead of this, if I remember correctly, there should be a proper way in
GNU Automake, to hook into 'all' ('all-local' maybe?). Untested:
all-local: libatomic.la
$(LIBTOOL) --mode=install $(INSTALL_DATA) libatomic.la
$(gcc_objdir)$(MULTISUBDIR)/
>> > 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
>> '[...]/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
>> `[...]/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.
Confirmed!
>> > --- 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.
Shouldn't it rather be alphabetically sorted with other '-fl[...]' flags?
Also, likewise move the actual option description, which currently is
located between '-fstrict-flex-arrays' and '-fsso-struct'.
I note this is in the 'C Language Options' section. Is that correct,
does '-flink-libatomic' only apply to C, or should this be moved into a
more general place? Likewise, then -- if applicable -- move it in
'gcc/common.opt' (where it, indeed, is generic, and not 'C' only in
'gcc/c-family/c.opt'?) and update 'gcc/common.opt.urls'.
If not only applicable to C, does moving into 'Linker Options' maybe make
sense?
Grüße
Thomas
