libatomic for s390x

[nealef]

Is your feature request related to a problem? Please describe.
Building node for s390x requires libatomic as the comment in v8.gyp reports:

       # Platforms that don't have Compare-And-Swap support need to link atomic
       # library to implement atomic memory access

Describe the solution you'd like
s390x has many atomic instructions including a few compare-and-swap instructions (differing on operand size).

Describe alternatives you've considered
I am curious as to which atomic operations are required and where in the code they are. I would look at adding them to eliminate the need for libatomic.

[bnoordhuis]

The file you're interested in is atomicops_internals_portable.h.

It's possible GCC knows how to emit instructions for some __atomic_*() intrinsincs. You can find out by removing -latomic from the link flags and checking the unresolved symbols (or rather: the symbols that are not unresolved.)

[nealef]

I took out the -latomic for s390x and I didn't get errors linking node.

--- a/tools/v8_gypfiles/v8.gyp  2019/04/24 14:31:16 1.1
+++ b/tools/v8_gypfiles/v8.gyp  2019/04/24 14:31:47
@@ -391,11 +391,11 @@
         }],
         # Platforms that don't have Compare-And-Swap support need to link atomic
         # library to implement atomic memory access
-        [ 'v8_current_cpu in ["mips", "mipsel", "mips64", "mips64el", "ppc", "ppc64", "s390", "s390x"]', {
+        [ 'v8_current_cpu in ["mips", "mipsel", "mips64", "mips64el", "ppc", "ppc64"]', {
             'link_settings': {
               'libraries': [ '-latomic', ],
             },
           },
         ],
         ['OS=="win"', {
           'msvs_precompiled_header': '<(V8_ROOT)/../../tools/msvs/pch/v8_pch.h',

However, I am getting these errors when I use devtoolset-6 and devtoolset-8 during the %install part of the RPM build.

DEBUG:   g++ -o /builddir/build/BUILD/node-v12.0.0/out/Release/mksnapshot -pthread -rdynamic -m64 -march=z196 -m64 -lstdc++ -Wl,--start-group /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/mksnapshot/deps/v8/src/snapshot/embedded-file-writer.o /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/mksnapshot/deps/v8/src/snapshot/mksnapshot.o /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_base.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_init.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_libbase.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_libplatform.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_nosnapshot.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/icu/libicui18n.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_libsampler.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/icu/libicuucx.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/icu/libicudata.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/icu/libicustubdata.a /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/tools/v8_gypfiles/libv8_initializers.a -ldl -lrt -Wl,--end-group
DEBUG: /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/v8_base/deps/v8/src/compiler/verifier.o: In function `v8::internal::compiler::ScheduleVerifier::Run(v8::internal::compiler::Schedule*)':
DEBUG: (.text._ZN2v88internal8compiler16ScheduleVerifier3RunEPNS1_8ScheduleE+0x216): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
DEBUG: (.text._ZN2v88internal8compiler16ScheduleVerifier3RunEPNS1_8ScheduleE+0xc28): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
DEBUG: /builddir/build/BUILD/node-v12.0.0/out/Release/obj.target/v8_base/deps/v8/src/elements.o: In function `v8::internal::(anonymous namespace)::FastPackedSmiElementsAccessor::~FastPackedSmiElementsAccessor()':
DEBUG: (.text._ZN2v88internal12_GLOBAL__N_129FastPackedSmiElementsAccessorD0Ev+0x12): undefined reference to `operator delete(void*, unsigned long)'

[mhdawson]

@sam-github can you comment as I know you ran into issues with libatomic.

[mhdawson]

@john-yan in case you can shed any additional insight as well.

[sam-github]

@nealef what compiler toolchain are you using when you don't get errors?

you may find select-compiler interesting, it shows how we build node for the various branches:

https://github.com/nodejs/build/blob/0aaa408e1816b55bf4ec75e4e79dbdf0e215dab3/jenkins/scripts/select-compiler.sh#L52-L80

I found libatomic necessary on centos7 ppc64, see nodejs/build@c6e9433

[nealef]

@sam-github I haven't built 12 cleanly yet as I was on a system with a gcc 4.8.5 toolchain so I tried both devtoolset-6 and devtoolset-8 where I got the same type of errors. I will check that link you provided.

[nealef]

@sam-github I think I found the problem. The RPM spec file has a . /opt/rh/devtoolset-8/enable in the %build but not in the %install.

[nealef]

@sam-github Confirm a clean build with devtoolset-8

[sam-github]

So, @nealef, you are finding that -latomic is not needed with devtoolset-8 on linux-s390x?

FYI, node builds with devtoolset-6 (EDIT: on 12.x), so it would have to be reconfirmed with that.

Also, while we are moving to using devtoolset when we can, it creates slightly more portable binaries, I'm not sure if we require people building from source to use it, we might also want to support gcc-6 (EDIT: on 12.x).

All of which is to say, does having -latomic on the link line cause problems? Are you requesting it be removed? If so, on what release lines, for what platforms?

[sam-github]

wrt.

The RPM spec file has a . /opt/rh/devtoolset-8/enable in the %build but not in the %install.

That sounds a bit like there might be an oddity elsewhere, either in node's install target, or in the rpm specfile. IIRC, the intention with rpm specfiles is that all building occur in the %build, so %install shouldn't require the toolset to be enabled. A workaround of always enabling the toolset doesn't seem so bad, but if something is broken, feel free to open a bug. Also, again just FYI, even though we use devtoolset, distro packagers aren't required to do so, they might have othe ways of dealing with binary compatibility. Also, when a binary is built for a specific distro, its not usual to want to copy it to another older version of the distro (or a completely different distroy), and run it. There has even been some cricism of devtoolset because it links code into the executable that is usually provided by shared libaries, preventing that code from being upgradeable by the distro in cases of bugs or sec problems. At a micro scale, this is similar to why distros link against shared openssl usually, instead of allowing node ot statically link its own. I don't know exactly what you are doing, so maybe none of this applies to you. YMMV.

[nealef]

@sam-github Confirm that -latomic is not required for s390/s390x.

The %install section does a make install that results in building stuff that is used by %check. devtoolset-8-libstdc++ uses the 4.8.5 shared object for most things but will statically link in specific functions that have been added.

Hopefully with RHEL 8 and then CentOS 8 the need to use devtoolset will be eliminated.

[sam-github]

Does %build do a make all?

[nealef]

No. The spec file came from the source tarball so I hadn't touched that bit.