Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: link libatomic on mac and linux #28232

Closed
wants to merge 1 commit into from

Conversation

@devsnek
Copy link
Member

commented Jun 14, 2019

Fixes #28231

cc @nodejs/build

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@devsnek devsnek added the build label Jun 14, 2019

@devsnek devsnek requested a review from addaleax Jun 14, 2019

@nodejs-github-bot

This comment has been minimized.

Copy link

commented Jun 14, 2019

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23892/

node.gyp Show resolved Hide resolved
@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2019

@nodejs/build macos failed with no library found for -latomic.

@rvagg

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@devsnek do you have any suggestions for actions for Build here? Sounds like a blocker if this won't pass on one the platforms you're intending to impact.

@addaleax

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@devsnek Are you building Node.js in a non-standard way (shared library or similar)? If this is already a dep for V8, I’m even more surprised that you’ve run into trouble…

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

I just pulled and ran make on a wsl debian install. i didn't configure it weirdly or anything.

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@devsnek can you provide more platform information, like gcc (I assume its gcc) version, and uname output?

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Linux xone 4.19.43-microsoft-standard #1 SMP Mon May 20 19:35:22 UTC 2019 x86_64 GNU/Linux and clang version 9.0.0

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

also a problem on a native (not wsl) linux i just set up (Linux xone 4.19.49-1-MANJARO #1 SMP PREEMPT Sun Jun 9 20:24:20 UTC 2019 x86_64 GNU/Linux, clang-8.0.0)

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

I'm going to guess the issue is Clang does not currently automatically link against libatomic when using libgcc_s. You may need to manually add -latomic to support this configuration when using non-native atomic operations (if you see link errors referring to __atomic_* functions). (https://clang.llvm.org/docs/Toolchain.html)

Perhaps we should get a linux clang ci machine?

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Perhaps we should get a linux clang ci machine?

Unless it comes with a volunteer to maintain it, I'm not sure that extending our build infrastructure is a good idea.

@nodejs/build Perhaps -latomic should be added to build platforms for all non-Windows platforms? I haven't done an investigation, but I believe this would make libatomic a build dep, even for toolchains that don't need it, which is a downside, but it could possibly make builds more consistent as an upside. One question to be answered would be if it was a build dep, would it become a runtime dep for binaries from toolchains that do not currently need libatomic? That would be backwards incompatible, so I assume a non-starter for 12.x, and maybe not a good idea for any version.

Also, its not clear to me - do we officially support clang? I'm sure we would make changes to allow people who want to use clang to be able, but I'm not sure its officially supported, to the point that someone from build would go through all our machines and update their ansibles, etc., to have libatomic (if that was required after adding -latomic to all builds).

Another question, for .gyp experts, is there a way to add linker flags conditionally based on the toolchain?

@nodejs-github-bot

This comment has been minimized.

@richardlau

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Another question, for .gyp experts, is there a way to add linker flags conditionally based on the toolchain?

Yes. We define llvm_version:

o['variables']['llvm_version'] = get_llvm_version(CC) if is_clang else 0

e.g. used to set cflags (but could be libraries as in this PR):

'conditions': [
[ 'llvm_version==0', {
'cflags': ['-Wno-old-style-declaration',],
}],

There's also a clang variable:

'clang%': 0,
(set in

node/common.gypi

Lines 112 to 114 in d2634be

['OS=="mac"', {
'clang%': 1,
}],
)

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I don't have the background to approve or reject, but seems like its failing to build on at least OS X:

osx/nodes/osx1011/out/Release/libv8_compiler.a /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/libv8_initializers.a -latomic -framework CoreFoundation -lm
09:55:55 ld: library not found for -latomic
@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

seems like this should be (OS == "linux" or OS == "mac") and llvm_version != 0?

@richardlau

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I guess the question is if this is needed on mac as the OS X builds have not (AFAIK) been failing and do use clang by default.

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

@richardlau i've needed it on my mac at least.

@devsnek devsnek force-pushed the fix-atomic-linkage branch from 8a6960b to 5fc6e13 Jun 18, 2019

@nodejs-github-bot

This comment has been minimized.

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

checking llvm_version seems to have worked!

@devsnek devsnek requested review from addaleax, richardlau and sam-github Jun 18, 2019

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

CI didn't pass, probably needs another kick (but I'm on poor wifi ATM, can't do).

@nodejs-github-bot

This comment has been minimized.

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

it's always parallel/test-worker-debug cc @joyeecheung

i am assuming the failure is unrelated to this change though.

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

this needs approvals for the changed semantics

devsnek added a commit to devsnek/node that referenced this pull request Jun 21, 2019
build: link libatomic on mac and linux
Fixes nodejs#28231

PR-URL: nodejs#28232
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@devsnek

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Landed in ab3174c

@devsnek devsnek closed this Jun 21, 2019

@devsnek devsnek deleted the fix-atomic-linkage branch Jun 21, 2019

targos added a commit that referenced this pull request Jul 2, 2019
build: link libatomic on mac and linux
Fixes #28231

PR-URL: #28232
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos referenced this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.