diff mbox series

[libstdc++,testsuite] Correct path to libatomic

Message ID CAGWvnykxVUecp9pO5iAhrXp7qhHi8Wh-TJPaH8-ADmFQ8zptVw@mail.gmail.com
State New
Headers show
Series [libstdc++,testsuite] Correct path to libatomic | expand

Commit Message

David Edelsohn April 24, 2021, 12:54 a.m. UTC
Some ports require libatomic for atomic operations, at least for some
data types and widths.  The libstdc++ testsuite previously was updated
to link against libatomic, but the search path was hard-coded to
something that is not always correct, and the shared library search
path was not set.

The search path was hard-coded to the expected location of the
libatomic build directory relative to the libstdc++ testsuite
directory, but if one uses parallelism when invoking the libstdc++
testsuite, the tests are run in the "normalXX" sub-directories, for
which the hard-coded search path is incorrect. The path also is
incorrect for alternative multilib and tool options.

This patch adopts the logic from gcc/testsuite/lib/atomic-dg.exp to
search for the library and adds the logic to the libstdc++ testsuite
libatomic seatch path code.  Previously the libstdc++ testsuite atomic
tests failed depending on the build configuration and if a build of
libatomic was installed in the default search path.

Bootstrapped on powerpc-ibm-aix7.2.3.0.

Okay to install?

Thanks, David

* testsuite/lib/dg-options.exp (atomic_link_flags): New.
(add_options_for_libatomic): Use atomic_link_flags.

     return $flags
 }

Comments

Jeff Law April 24, 2021, 4:12 a.m. UTC | #1
On 4/23/2021 6:54 PM, David Edelsohn via Gcc-patches wrote:
> Some ports require libatomic for atomic operations, at least for some
> data types and widths.  The libstdc++ testsuite previously was updated
> to link against libatomic, but the search path was hard-coded to
> something that is not always correct, and the shared library search
> path was not set.
>
> The search path was hard-coded to the expected location of the
> libatomic build directory relative to the libstdc++ testsuite
> directory, but if one uses parallelism when invoking the libstdc++
> testsuite, the tests are run in the "normalXX" sub-directories, for
> which the hard-coded search path is incorrect. The path also is
> incorrect for alternative multilib and tool options.
>
> This patch adopts the logic from gcc/testsuite/lib/atomic-dg.exp to
> search for the library and adds the logic to the libstdc++ testsuite
> libatomic seatch path code.  Previously the libstdc++ testsuite atomic
> tests failed depending on the build configuration and if a build of
> libatomic was installed in the default search path.
>
> Bootstrapped on powerpc-ibm-aix7.2.3.0.
>
> Okay to install?
>
> Thanks, David
>
> * testsuite/lib/dg-options.exp (atomic_link_flags): New.
> (add_options_for_libatomic): Use atomic_link_flags.

OK

jeff
Jonathan Wakely April 26, 2021, 11:19 a.m. UTC | #2
On 23/04/21 20:54 -0400, David Edelsohn via Libstdc++ wrote:
>Some ports require libatomic for atomic operations, at least for some
>data types and widths.  The libstdc++ testsuite previously was updated
>to link against libatomic, but the search path was hard-coded to
>something that is not always correct, and the shared library search
>path was not set.

In libstdc++_init we have:

     # Locate libatomic.
     set v3-libatomic 0
     set libatomicdir [lookfor_file $blddir/../libatomic .libs/libatomic.$shlib_ext]
     if {$libatomicdir != ""} {
	set v3-libatomic 1
	set libatomicdir [file dirname $libatomicdir]
	append ld_library_path_tmp ":${libatomicdir}"
	verbose -log "libatomic support detected"
     }
     v3track libatomicdir 3

Which should have added it to the search path unconditionally, so that
it will be found if a test is linked with -latomic and needs it. I
Don't know why that's not working for AIX, I see it doing the right
thing in the logs for x86_64-linux (and the path is right for the
alternative unix/-m32 multilib tests):

libatomic support detected
libgomp support detected
shared library support detected
LD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
LD_RUN_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
SHLIB_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
LD_LIBRARY_PATH_32=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
LD_LIBRARY_PATH_64=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
DYLD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
LD_LIBRARY_PATH = :/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32

>The search path was hard-coded to the expected location of the
>libatomic build directory relative to the libstdc++ testsuite
>directory, but if one uses parallelism when invoking the libstdc++
>testsuite, the tests are run in the "normalXX" sub-directories, for
>which the hard-coded search path is incorrect.

This has occurred to me previously, and I checked it, and it works
fine for parallel test runs. I must be running the tests differently
from you, as I can't reproruce the problem.

For example, in r11-8285 I added the libatomic options to
30_threads/semaphore/try_acquire_posix.cc because it was failing on
AIX (on gcc119 specifically). With the { dg-add-options libatomic } in
place it no longer fails. That is true for parallel and non-parallel
test runs, and whether I run "make check" in the $target/libstdc++-v3
directory, or $target/libstdc++-v3/testsuite, or run make
check-target-libstdc++-v3 in the top-level build dir.

>The path also is
>incorrect for alternative multilib and tool options.

Is it?  For alternative multilibs we have something like:

./libstdc++/testsuite
./libatomic/.libs
ppc64/libstdc++-v3/testsuite
ppc64/libatomic/.libs
pthread/libstdc++-v3/testsuite
pthread/libatomic/.libs

In each case, the relative path from the v3/testsuite directory to
libatomic.a ia the same, ../../libatomic/.src

The patch seems like an improvement, but I don't see the problem it
was fixing.

>This patch adopts the logic from gcc/testsuite/lib/atomic-dg.exp to
>search for the library and adds the logic to the libstdc++ testsuite
>libatomic seatch path code.  Previously the libstdc++ testsuite atomic
>tests failed depending on the build configuration and if a build of
>libatomic was installed in the default search path.
>
>Bootstrapped on powerpc-ibm-aix7.2.3.0.
>
>Okay to install?
>
>Thanks, David
>
>* testsuite/lib/dg-options.exp (atomic_link_flags): New.
>(add_options_for_libatomic): Use atomic_link_flags.
>
>--- a/libstdc++-v3/testsuite/lib/dg-options.exp
>+++ b/libstdc++-v3/testsuite/lib/dg-options.exp
>@@ -260,13 +260,58 @@ proc add_options_for_net_ts { flags } {
> # Add to FLAGS all the target-specific flags to link to libatomic,
> # if required for atomics on pointers and 64-bit types.
>
>+proc atomic_link_flags { paths } {
>+    global srcdir
>+    global ld_library_path
>+    global shlib_ext
>+
>+    set gccpath ${paths}
>+    set flags ""
>+
>+    set shlib_ext [get_shlib_extension]
>+
>+    if { $gccpath != "" } {
>+      if { [file exists "${gccpath}/libatomic/.libs/libatomic.a"]
>+           || [file exists "${gccpath}/libatomic/.libs/libatomic.${shlib_ext}"]
> } {
>+          append flags " -B${gccpath}/libatomic/ "
>+          append flags " -L${gccpath}/libatomic/.libs"
>+          append ld_library_path ":${gccpath}/libatomic/.libs"
>+      }
>+    } else {
>+      global tool_root_dir
>+
>+      set libatomic [lookfor_file ${tool_root_dir} libatomic]
>+      if { $libatomic != "" } {
>+          append flags "-L${libatomic} "
>+          append ld_library_path ":${libatomic}"
>+      }
>+    }
>+
>+    set_ld_library_path_env_vars
>+
>+    return "$flags"
>+}
>+
> proc add_options_for_libatomic { flags } {
>     if { [istarget hppa*-*-hpux*]
>         || ([istarget powerpc*-*-*] && [check_effective_target_ilp32])
>         || [istarget riscv*-*-*]
>         || ([istarget sparc*-*-linux-gnu] && [check_effective_target_ilp32])
>        } {
>-       return "$flags -L../../libatomic/.libs -latomic"
>+       global TOOL_OPTIONS
>+
>+       set link_flags ""
>+       if ![is_remote host] {
>+           if [info exists TOOL_OPTIONS] {
>+               set link_flags "[atomic_link_flags [get_multilibs
>${TOOL_OPTIONS}]]"
>+           } else {
>+               set link_flags "[atomic_link_flags [get_multilibs]]"
>+           }
>+       }
>+
>+       append link_flags " -latomic "
>+
>+       return "$flags $link_flags"
>     }
>     return $flags
> }
>
David Edelsohn April 26, 2021, 1:17 p.m. UTC | #3
On Mon, Apr 26, 2021 at 7:19 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 23/04/21 20:54 -0400, David Edelsohn via Libstdc++ wrote:
> >Some ports require libatomic for atomic operations, at least for some
> >data types and widths.  The libstdc++ testsuite previously was updated
> >to link against libatomic, but the search path was hard-coded to
> >something that is not always correct, and the shared library search
> >path was not set.
>
> In libstdc++_init we have:
>
>      # Locate libatomic.
>      set v3-libatomic 0
>      set libatomicdir [lookfor_file $blddir/../libatomic .libs/libatomic.$shlib_ext]
>      if {$libatomicdir != ""} {
>         set v3-libatomic 1
>         set libatomicdir [file dirname $libatomicdir]
>         append ld_library_path_tmp ":${libatomicdir}"
>         verbose -log "libatomic support detected"
>      }
>      v3track libatomicdir 3
>
> Which should have added it to the search path unconditionally, so that
> it will be found if a test is linked with -latomic and needs it. I
> Don't know why that's not working for AIX, I see it doing the right
> thing in the logs for x86_64-linux (and the path is right for the
> alternative unix/-m32 multilib tests):
>
> libatomic support detected
> libgomp support detected
> shared library support detected
> LD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> LD_RUN_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> SHLIB_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> LD_LIBRARY_PATH_32=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> LD_LIBRARY_PATH_64=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> DYLD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> LD_LIBRARY_PATH = :/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>
> >The search path was hard-coded to the expected location of the
> >libatomic build directory relative to the libstdc++ testsuite
> >directory, but if one uses parallelism when invoking the libstdc++
> >testsuite, the tests are run in the "normalXX" sub-directories, for
> >which the hard-coded search path is incorrect.
>
> This has occurred to me previously, and I checked it, and it works
> fine for parallel test runs. I must be running the tests differently
> from you, as I can't reproruce the problem.
>
> For example, in r11-8285 I added the libatomic options to
> 30_threads/semaphore/try_acquire_posix.cc because it was failing on
> AIX (on gcc119 specifically). With the { dg-add-options libatomic } in
> place it no longer fails. That is true for parallel and non-parallel
> test runs, and whether I run "make check" in the $target/libstdc++-v3
> directory, or $target/libstdc++-v3/testsuite, or run make
> check-target-libstdc++-v3 in the top-level build dir.
>
> >The path also is
> >incorrect for alternative multilib and tool options.
>
> Is it?  For alternative multilibs we have something like:
>
> ./libstdc++/testsuite
> ./libatomic/.libs
> ppc64/libstdc++-v3/testsuite
> ppc64/libatomic/.libs
> pthread/libstdc++-v3/testsuite
> pthread/libatomic/.libs
>
> In each case, the relative path from the v3/testsuite directory to
> libatomic.a ia the same, ../../libatomic/.src
>
> The patch seems like an improvement, but I don't see the problem it
> was fixing.

The testsuite directory in the build tree would be

./libstdc++-v3/testsuite

and the original libatomic search path support unilaterally would add

../../libatomic/.libs

to the link path.

But if one runs the testsuite with parallelism, the testsuite adds an
additional level of subdirectories, e.g.,

./libstdc++-v3/testsuite/normal1
./libstdc++-v3/testsuite/normal2
./libstdc++-v3/testsuite/normal3

in which case ../../libatomic is not the correct path and "../.."
doesn't point into the root of the target libraries directory.
Depending on whether the testsuite is invoked with or without
parallelism, one needs an additional ".." in the libatomic path.  The
patch uses the same logic as other parts of the testsuite to determine
the path relative to the gcc build directory, including the
TOOL_OPTIONS.

Thanks, David
Jonathan Wakely April 26, 2021, 3:50 p.m. UTC | #4
On 26/04/21 09:17 -0400, David Edelsohn via Libstdc++ wrote:
>On Mon, Apr 26, 2021 at 7:19 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> On 23/04/21 20:54 -0400, David Edelsohn via Libstdc++ wrote:
>> >Some ports require libatomic for atomic operations, at least for some
>> >data types and widths.  The libstdc++ testsuite previously was updated
>> >to link against libatomic, but the search path was hard-coded to
>> >something that is not always correct, and the shared library search
>> >path was not set.
>>
>> In libstdc++_init we have:
>>
>>      # Locate libatomic.
>>      set v3-libatomic 0
>>      set libatomicdir [lookfor_file $blddir/../libatomic .libs/libatomic.$shlib_ext]
>>      if {$libatomicdir != ""} {
>>         set v3-libatomic 1
>>         set libatomicdir [file dirname $libatomicdir]
>>         append ld_library_path_tmp ":${libatomicdir}"
>>         verbose -log "libatomic support detected"
>>      }
>>      v3track libatomicdir 3
>>
>> Which should have added it to the search path unconditionally, so that
>> it will be found if a test is linked with -latomic and needs it. I
>> Don't know why that's not working for AIX, I see it doing the right
>> thing in the logs for x86_64-linux (and the path is right for the
>> alternative unix/-m32 multilib tests):
>>
>> libatomic support detected
>> libgomp support detected
>> shared library support detected
>> LD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>> LD_RUN_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>> SHLIB_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>> LD_LIBRARY_PATH_32=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>> LD_LIBRARY_PATH_64=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>> DYLD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>> LD_LIBRARY_PATH = :/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
>>
>> >The search path was hard-coded to the expected location of the
>> >libatomic build directory relative to the libstdc++ testsuite
>> >directory, but if one uses parallelism when invoking the libstdc++
>> >testsuite, the tests are run in the "normalXX" sub-directories, for
>> >which the hard-coded search path is incorrect.
>>
>> This has occurred to me previously, and I checked it, and it works
>> fine for parallel test runs. I must be running the tests differently
>> from you, as I can't reproruce the problem.
>>
>> For example, in r11-8285 I added the libatomic options to
>> 30_threads/semaphore/try_acquire_posix.cc because it was failing on
>> AIX (on gcc119 specifically). With the { dg-add-options libatomic } in
>> place it no longer fails. That is true for parallel and non-parallel
>> test runs, and whether I run "make check" in the $target/libstdc++-v3
>> directory, or $target/libstdc++-v3/testsuite, or run make
>> check-target-libstdc++-v3 in the top-level build dir.
>>
>> >The path also is
>> >incorrect for alternative multilib and tool options.
>>
>> Is it?  For alternative multilibs we have something like:
>>
>> ./libstdc++/testsuite
>> ./libatomic/.libs
>> ppc64/libstdc++-v3/testsuite
>> ppc64/libatomic/.libs
>> pthread/libstdc++-v3/testsuite
>> pthread/libatomic/.libs
>>
>> In each case, the relative path from the v3/testsuite directory to
>> libatomic.a ia the same, ../../libatomic/.src
>>
>> The patch seems like an improvement, but I don't see the problem it
>> was fixing.
>
>The testsuite directory in the build tree would be
>
>./libstdc++-v3/testsuite
>
>and the original libatomic search path support unilaterally would add
>
>../../libatomic/.libs
>
>to the link path.
>
>But if one runs the testsuite with parallelism, the testsuite adds an
>additional level of subdirectories, e.g.,
>
>./libstdc++-v3/testsuite/normal1
>./libstdc++-v3/testsuite/normal2
>./libstdc++-v3/testsuite/normal3

Right, I understand the problem in theory (though I still question
whether it's even theoretically a problem for multilib paths), but my
point is that I don't see a problem in practice. It works fine. So I'm
curious why it works for me and not for you.

I've never seen this cause a problem on any target, with any level of
parallelism.

>in which case ../../libatomic is not the correct path and "../.."
>doesn't point into the root of the target libraries directory.
>Depending on whether the testsuite is invoked with or without
>parallelism, one needs an additional ".." in the libatomic path.  The
>patch uses the same logic as other parts of the testsuite to determine
>the path relative to the gcc build directory, including the
>TOOL_OPTIONS.

All you've done is restate the description of the patch. I read it. I
understand it. Please re-read my email. I'm not asking what the patch
does, I'm trying to find out why you need the patch and I don't.

It turns out that on gcc119 -L wrong-path/libatomic -latomic is
causing the tests to link to /usr/lib/libatomic.a (and ignoring the -L
option). So that answers my question. I wasn't seeing it because some
other libatomic was being found when the -L path was wrong.
David Edelsohn April 26, 2021, 4:21 p.m. UTC | #5
On Mon, Apr 26, 2021 at 11:50 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 26/04/21 09:17 -0400, David Edelsohn via Libstdc++ wrote:
> >On Mon, Apr 26, 2021 at 7:19 AM Jonathan Wakely <jwakely@redhat.com> wrote:
> >>
> >> On 23/04/21 20:54 -0400, David Edelsohn via Libstdc++ wrote:
> >> >Some ports require libatomic for atomic operations, at least for some
> >> >data types and widths.  The libstdc++ testsuite previously was updated
> >> >to link against libatomic, but the search path was hard-coded to
> >> >something that is not always correct, and the shared library search
> >> >path was not set.
> >>
> >> In libstdc++_init we have:
> >>
> >>      # Locate libatomic.
> >>      set v3-libatomic 0
> >>      set libatomicdir [lookfor_file $blddir/../libatomic .libs/libatomic.$shlib_ext]
> >>      if {$libatomicdir != ""} {
> >>         set v3-libatomic 1
> >>         set libatomicdir [file dirname $libatomicdir]
> >>         append ld_library_path_tmp ":${libatomicdir}"
> >>         verbose -log "libatomic support detected"
> >>      }
> >>      v3track libatomicdir 3
> >>
> >> Which should have added it to the search path unconditionally, so that
> >> it will be found if a test is linked with -latomic and needs it. I
> >> Don't know why that's not working for AIX, I see it doing the right
> >> thing in the logs for x86_64-linux (and the path is right for the
> >> alternative unix/-m32 multilib tests):
> >>
> >> libatomic support detected
> >> libgomp support detected
> >> shared library support detected
> >> LD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >> LD_RUN_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >> SHLIB_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >> LD_LIBRARY_PATH_32=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >> LD_LIBRARY_PATH_64=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >> DYLD_LIBRARY_PATH=:/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >> LD_LIBRARY_PATH = :/home/jwakely/src/gcc/build/gcc:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libatomic/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/../libgomp/.libs:/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/32/libstdc++-v3/src/.libs:/home/jwakely/src/gcc/build/gcc/32
> >>
> >> >The search path was hard-coded to the expected location of the
> >> >libatomic build directory relative to the libstdc++ testsuite
> >> >directory, but if one uses parallelism when invoking the libstdc++
> >> >testsuite, the tests are run in the "normalXX" sub-directories, for
> >> >which the hard-coded search path is incorrect.
> >>
> >> This has occurred to me previously, and I checked it, and it works
> >> fine for parallel test runs. I must be running the tests differently
> >> from you, as I can't reproruce the problem.
> >>
> >> For example, in r11-8285 I added the libatomic options to
> >> 30_threads/semaphore/try_acquire_posix.cc because it was failing on
> >> AIX (on gcc119 specifically). With the { dg-add-options libatomic } in
> >> place it no longer fails. That is true for parallel and non-parallel
> >> test runs, and whether I run "make check" in the $target/libstdc++-v3
> >> directory, or $target/libstdc++-v3/testsuite, or run make
> >> check-target-libstdc++-v3 in the top-level build dir.
> >>
> >> >The path also is
> >> >incorrect for alternative multilib and tool options.
> >>
> >> Is it?  For alternative multilibs we have something like:
> >>
> >> ./libstdc++/testsuite
> >> ./libatomic/.libs
> >> ppc64/libstdc++-v3/testsuite
> >> ppc64/libatomic/.libs
> >> pthread/libstdc++-v3/testsuite
> >> pthread/libatomic/.libs
> >>
> >> In each case, the relative path from the v3/testsuite directory to
> >> libatomic.a ia the same, ../../libatomic/.src
> >>
> >> The patch seems like an improvement, but I don't see the problem it
> >> was fixing.
> >
> >The testsuite directory in the build tree would be
> >
> >./libstdc++-v3/testsuite
> >
> >and the original libatomic search path support unilaterally would add
> >
> >../../libatomic/.libs
> >
> >to the link path.
> >
> >But if one runs the testsuite with parallelism, the testsuite adds an
> >additional level of subdirectories, e.g.,
> >
> >./libstdc++-v3/testsuite/normal1
> >./libstdc++-v3/testsuite/normal2
> >./libstdc++-v3/testsuite/normal3
>
> Right, I understand the problem in theory (though I still question
> whether it's even theoretically a problem for multilib paths), but my
> point is that I don't see a problem in practice. It works fine. So I'm
> curious why it works for me and not for you.
>
> I've never seen this cause a problem on any target, with any level of
> parallelism.
>
> >in which case ../../libatomic is not the correct path and "../.."
> >doesn't point into the root of the target libraries directory.
> >Depending on whether the testsuite is invoked with or without
> >parallelism, one needs an additional ".." in the libatomic path.  The
> >patch uses the same logic as other parts of the testsuite to determine
> >the path relative to the gcc build directory, including the
> >TOOL_OPTIONS.
>
> All you've done is restate the description of the patch. I read it. I
> understand it. Please re-read my email. I'm not asking what the patch
> does, I'm trying to find out why you need the patch and I don't.
>
> It turns out that on gcc119 -L wrong-path/libatomic -latomic is
> causing the tests to link to /usr/lib/libatomic.a (and ignoring the -L
> option). So that answers my question. I wasn't seeing it because some
> other libatomic was being found when the -L path was wrong.

Most targets do not need libatomic.  libatomic only is added to the
link libraries for a few, specific targets (hpux, ppc32, riscv,
sparc32).  And the testcases work if an older version of libatomic is
installed in the search path.  If I configure GCC with a prefix where
I have libatomic already installed, it works.

Thanks, David
diff mbox series

Patch

--- a/libstdc++-v3/testsuite/lib/dg-options.exp
+++ b/libstdc++-v3/testsuite/lib/dg-options.exp
@@ -260,13 +260,58 @@  proc add_options_for_net_ts { flags } {
 # Add to FLAGS all the target-specific flags to link to libatomic,
 # if required for atomics on pointers and 64-bit types.

+proc atomic_link_flags { paths } {
+    global srcdir
+    global ld_library_path
+    global shlib_ext
+
+    set gccpath ${paths}
+    set flags ""
+
+    set shlib_ext [get_shlib_extension]
+
+    if { $gccpath != "" } {
+      if { [file exists "${gccpath}/libatomic/.libs/libatomic.a"]
+           || [file exists "${gccpath}/libatomic/.libs/libatomic.${shlib_ext}"]
 } {
+          append flags " -B${gccpath}/libatomic/ "
+          append flags " -L${gccpath}/libatomic/.libs"
+          append ld_library_path ":${gccpath}/libatomic/.libs"
+      }
+    } else {
+      global tool_root_dir
+
+      set libatomic [lookfor_file ${tool_root_dir} libatomic]
+      if { $libatomic != "" } {
+          append flags "-L${libatomic} "
+          append ld_library_path ":${libatomic}"
+      }
+    }
+
+    set_ld_library_path_env_vars
+
+    return "$flags"
+}
+
 proc add_options_for_libatomic { flags } {
     if { [istarget hppa*-*-hpux*]
         || ([istarget powerpc*-*-*] && [check_effective_target_ilp32])
         || [istarget riscv*-*-*]
         || ([istarget sparc*-*-linux-gnu] && [check_effective_target_ilp32])
        } {
-       return "$flags -L../../libatomic/.libs -latomic"
+       global TOOL_OPTIONS
+
+       set link_flags ""
+       if ![is_remote host] {
+           if [info exists TOOL_OPTIONS] {
+               set link_flags "[atomic_link_flags [get_multilibs
${TOOL_OPTIONS}]]"
+           } else {
+               set link_flags "[atomic_link_flags [get_multilibs]]"
+           }
+       }
+
+       append link_flags " -latomic "
+
+       return "$flags $link_flags"
     }