build: make linker checks more robust #17874

fanquake · 2020-01-05T07:07:45Z

Check for a flag to turn linker warnings into errors. When flags are passed to
linkers via the compiler driver using a -Wl,-foo flag, linker warnings may be
swallowed rather than bubbling up.

This is one of Corys commits that I've modified to also add -Wl,-fatal_warnings
for darwin.

DrahtBot · 2020-01-05T12:13:01Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove fdelt_chk back-compat code and sanity check #18862 (Remove fdelt_chk back-compat code and sanity check by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2020-01-06T03:59:03Z

Gitian builds

File	commit `b949ac9` (master)	commit `996e3f2` (master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz	`a14207517d25fece...`	`2d5d8f78231bd88f...`
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz	`51fd58ca7f14c4bb...`	`fd5ccd9659473979...`
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz	`0c13785557c412cb...`	`24380e11e1fdb5c8...`
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz	`d717d387941a6f52...`	`d71c9392a5801a54...`
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz	`485858210efe05c2...`	`d138a1306b8843d3...`
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz	`69a1cf3b9bf79fbe...`	`72a461b011f30050...`
bitcoin-0.19.99-osx-unsigned.dmg	`c876fb9db3d4d189...`	`b9e42fecdeea2c8e...`
bitcoin-0.19.99-osx64.tar.gz	`dd8a3ebd540cc66c...`	`12d5b37d851bf4ac...`
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz	`6642fc1c8d1635d1...`	`0bfd90a99ce431ee...`
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz	`6cccb448a4262e7b...`	`0651c0db0caf6b2c...`
bitcoin-0.19.99-win64-debug.zip	`f94ba1033e8cc317...`	`8acc4fd954f986a3...`
bitcoin-0.19.99-win64-setup-unsigned.exe	`8b59c3214bfb6a46...`	`0bef0f106215769e...`
bitcoin-0.19.99-win64.zip	`a23d7c88ff460d78...`	`8f793e66ae312afb...`
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz	`62a5918ebf22fc21...`	`596cd0c079f17593...`
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz	`98cd19fe59bc6eff...`	`15937a64b1c2979c...`
bitcoin-0.19.99.tar.gz	`5a211d09899b034c...`	`f885c34c75532d83...`
bitcoin-core-linux-0.20-res.yml	`7ed024b81ed9b639...`	`9abdfb9cc40ea498...`
bitcoin-core-osx-0.20-res.yml	`5947a980f2b74084...`	`8a8d6d4f4dceed25...`
bitcoin-core-win-0.20-res.yml	`9e64f2d02f143a21...`	`95dfb8ad7aabb731...`
linux-build.log	`ef1f26008e10e842...`	`e45f60189d89a2ce...`
osx-build.log	`4ab5188b79e2e50c...`	`7ebe457936033f70...`
win-build.log	`0567536136b41af8...`	`4982c2f710176ddc...`
bitcoin-core-linux-0.20-res.yml.diff		`ae47fd004452fa09...`
bitcoin-core-osx-0.20-res.yml.diff		`a94472bfb55530fd...`
bitcoin-core-win-0.20-res.yml.diff		`ab3897018163b920...`
linux-build.log.diff		`e0144ec71849b6cf...`
osx-build.log.diff		`794e9f50b5549bd6...`
win-build.log.diff		`6f48a8ab3566aecb...`

laanwj · 2020-01-20T19:59:22Z

Concept ACK

DrahtBot · 2020-02-06T08:34:21Z

Needs rebase

DrahtBot · 2020-02-10T12:03:36Z

Needs rebase

dongcarl · 2020-03-09T20:37:06Z

Going to restate my understanding just to make sure I'm clear on the logic here:

The AX_CHECK_LINK_FLAG macro checks whether a linker flag exists by invoking the linker, and defines failures as when the linker "gives an error", however, warnings are ignored. Since it is possible that when the AX_CHECK_LINK_FLAG macro invokes the linker with a flag it doesn't understand, the linker only gives a warning, we should make sure this doesn't happen by turning warnings into errors, forcing the failure case.

@fanquake does that sound right?

hebasto · 2020-03-10T11:41:51Z

Concept ACK.

fanquake · 2020-03-11T03:53:10Z

@fanquake does that sound right?

@dongcarl Thanks for restating. I didn't like how I'd split the addition of the flag up, so I've pushed a new version of this change where they are together.

What this currently does is adding (either variant of) -fatal_warnings to the linker flags for all subsequent AX_CHECK_LINK_FLAG calls. -fatal_warnings does not get added to LDFLAGS and thus is not used at link time.

The practical affect of this change is that linker flags which previously would have been tested, thrown a warning, and still been added to LDFLAGS, are now no-longer being added.

For example, if we were to add an obsoleted macOS linker flag, such as -nofixprebinding, to configure.ac we'd get this behaviour:

Master (`b5c7665`)

AX_CHECK_LINK_FLAG([[-Wl,-nofixprebinding]]....
# configure checks and adds to $LDFLAGS
checking whether the linker accepts -Wl,-nofixprebinding... yes
# link time
ld: warning: option -nofixprebinding is obsolete and being ignored

config.log:

configure:18716: checking whether the linker accepts -Wl,-nofixprebinding
configure:18735: g++ -std=c++11 -o conftest -g -O2    -Wl,-nofixprebinding conftest.cpp  >&5
ld: warning: option -nofixprebinding is obsolete and being ignored
configure:18735: $? = 0
configure:18745: result: yes

This PR (`d333883`)

# configure check fails
checking whether the linker accepts -Wl,-nofixprebinding... no

config.log:

configure:18752: checking whether the linker accepts -Wl,-nofixprebinding
configure:18771: g++ -std=c++11 -o conftest -g -O2   -Wl,-fatal_warnings -Wl,-nofixprebinding conftest.cpp  >&5
ld: warning: option -nofixprebinding is obsolete and being ignored
ld: fatal warning(s) induced error (-fatal_warnings)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Note

For master, using an option that would otherwise cause an error, such as a linker flag that doesn't exist, is already a failure case. i.e:

AX_CHECK_LINK_FLAG([[-Wl,-doesnt_exist]]...
checking whether the linker accepts -Wl,-doesnt_exist... no

config.log:

configure:18716: checking whether the linker accepts -Wl,-doesnt_exist
configure:18735: g++ -std=c++11 -o conftest -g -O2    -Wl,-doesnt_exist conftest.cpp  >&5
ld: unknown option: -doesnt_exist
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Is this inline with your understanding?

One other thought I had is, somewhat similar to how we have an option for-Werror maybe we'd like to have a way for users to opt into passing -fatal_warnings at linker time?

laanwj · 2020-03-11T15:23:29Z

configure.ac

+    AX_CHECK_LINK_FLAG([-Wl,-fatal_warnings],[LDFLAG_WERROR="-Wl,-fatal_warnings"],[LDFLAG_WERROR=""])
+    ;;
+  *)
+    AX_CHECK_LINK_FLAG([-Wl,--fatal-warnings],[LDFLAG_WERROR="-Wl,--fatal-warnings"],[LDFLAG_WERROR=""])


Very subtle difference, was about to respond 'these two cases are the same' then noticed - versus -- option prefix 🤦‍♀️

hebasto · 2020-03-18T12:18:25Z

From the AX_CHECK_LINK_FLAG docs:

Warnings, however, are ignored

If EXTRA-FLAGS is defined, it is added to the linker’s default flags when the check is done. The check is thus made with the flags: "LDFLAGS EXTRA-FLAGS FLAG". This can for example be used to force the linker to issue an error when a bad flag is given.

@dongcarl

The AX_CHECK_LINK_FLAG macro checks whether a linker flag exists by invoking the linker, and defines failures as when the linker "gives an error", however, warnings are ignored. Since it is possible that when the AX_CHECK_LINK_FLAG macro invokes the linker with a flag it doesn't understand, the linker only gives a warning, we should make sure this doesn't happen by turning warnings into errors, forcing the failure case.

It seems when the AX_CHECK_LINK_FLAG macro invokes the linker with a flag it doesn't understand it just runs ACTION-FAILURE.

The only case when a warning could be raised is passing a valid but obsolete options, right?

I've tested this pr suggestion on macOS 10.15.3 with an obsolete option -nofixprebinding. It works as expected.

But I could not test on Linux Mint 19.3 because I did not find any mention of obsolete options in the ld docs. Did I miss smth?

@fanquake

One other thought I had is, somewhat similar to how we have an option for-Werror maybe we'd like to have a way for users to opt into passing -fatal_warnings at linker time?

Passing -fatal_warnings at linker time could improve build robustness, IMHO.

cd24f37 doc: Better explain GNU ld's dislike of ld64's options (fanquake) Pull request description: There's also now more than a single option being special cased for darwin. If we didn't special case these options they would still end up on the link line and the binaries produced would just segfault. I'm going to plug #17874 here as well, because adding `-fatal-warnings` to our `AX_CHECK_LINK_FLAG` calls would mostly prevent this sort of option mangling from happening. An example of the warning behaviour: ```bash echo "int main() {}" | g++ -x c++ -std=c++11 -Wl,-dead_strip - /usr/bin/ld: warning: cannot find entry symbol ad_strip; defaulting to 0000000000001040 nm -C a.out 0000000000001000 t _init 0000000000001040 T _start U ad_strip ``` ACKs for top commit: dongcarl: ACK cd24f37 Tree-SHA512: 8c5ff11b647e7d44dbb3f509a07caf8606a6b481c114403f0de72b3ad65395dbe9a3436e731ae1b46a823431ed23c3c6aacab8942d78629d59cd8c258c5dbf02

vasild · 2020-05-06T09:07:20Z

ACK d333883

So, the semantics of this are different than those of compilation warnings (e.g. -Wfoo or -Werror=bar) - with compilation warnings flags during configure time we check whether a given flag is supported and if yes, then we use it during make time. With -Wl,--fatal-warnings, if it is available, we only append it to the linker flags when checking whether other linker flags like -Wl,baz are supported.

So, with this PR, we are not going to use -Wl,baz if it is going to produce a warning. And
-Wl,--fatal-warnings is not supposed to be used during make time by this PR.

I tested this on FreeBSD 12 and confirm it works as expected - it is being used when checking whether other linker flags are supported. No linker flags are being excluded due to it - before and after the PR LDFLAGS is -pthread -Wl,-z,relro -Wl,-z,now -pie -L/usr/local/lib.

This PR omitted the following, I guess on purpose?

if test x$enable_determinism = xyes; then
  if test x$TARGET_OS = xwindows; then
    AX_CHECK_LINK_FLAG([[-Wl,--no-insert-timestamp]], [LDFLAGS="$LDFLAGS -Wl,--no-insert-timestamp"])
  fi
fi

Check for a flag to turn linker warnings into errors. When flags are passed to linkers via the compiler driver using a -Wl,-foo flag, linker warnings may be swallowed rather than bubbling up. Co-authored-by: fanquake <[email protected]>

fanquake · 2020-05-06T09:41:00Z

Thanks for the review.

This PR omitted the following, I guess on purpose?

Nope. I've added that to configure.ac in the interim, and hadn't updated this PR. Good catch. Have pushed the fix.

vasild · 2020-05-06T10:34:22Z

re-ACK 03da4c7

My thought was "this is something Windows-specific, I do not want to know about it"...

…ptions cd24f37 doc: Better explain GNU ld's dislike of ld64's options (fanquake) Pull request description: There's also now more than a single option being special cased for darwin. If we didn't special case these options they would still end up on the link line and the binaries produced would just segfault. I'm going to plug bitcoin#17874 here as well, because adding `-fatal-warnings` to our `AX_CHECK_LINK_FLAG` calls would mostly prevent this sort of option mangling from happening. An example of the warning behaviour: ```bash echo "int main() {}" | g++ -x c++ -std=c++11 -Wl,-dead_strip - /usr/bin/ld: warning: cannot find entry symbol ad_strip; defaulting to 0000000000001040 nm -C a.out 0000000000001000 t _init 0000000000001040 T _start U ad_strip ``` ACKs for top commit: dongcarl: ACK cd24f37 Tree-SHA512: 8c5ff11b647e7d44dbb3f509a07caf8606a6b481c114403f0de72b3ad65395dbe9a3436e731ae1b46a823431ed23c3c6aacab8942d78629d59cd8c258c5dbf02

fanquake added Build system Needs gitian build labels Jan 5, 2020

fanquake requested a review from dongcarl January 5, 2020 07:07

DrahtBot removed the Needs gitian build label Jan 6, 2020

fanquake mentioned this pull request Jan 7, 2020

build: various build system improvements #17768

Closed

9 tasks

kangzixiang approved these changes Jan 14, 2020

View reviewed changes

DrahtBot added the Needs rebase label Feb 6, 2020

fanquake force-pushed the make_linker_checks_more_robust branch from 8b048b6 to 7963904 Compare February 6, 2020 08:47

fanquake removed the Needs rebase label Feb 6, 2020

DrahtBot added the Needs rebase label Feb 10, 2020

fanquake force-pushed the make_linker_checks_more_robust branch from 7963904 to feecd9f Compare February 10, 2020 12:08

fanquake removed the Needs rebase label Feb 10, 2020

fanquake force-pushed the make_linker_checks_more_robust branch from feecd9f to d3ddae6 Compare March 9, 2020 01:40

fanquake force-pushed the make_linker_checks_more_robust branch from d3ddae6 to d333883 Compare March 11, 2020 03:49

laanwj reviewed Mar 11, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 17, 2020

gitian-linux: Build binaries for 64-bit POWER #14066

Closed

fanquake mentioned this pull request Apr 27, 2020

doc: Better explain GNU ld's dislike of ld64's options #18779

Merged

DrahtBot mentioned this pull request May 4, 2020

Remove fdelt_chk back-compat code and sanity check #18862

Merged

vasild approved these changes May 6, 2020

View reviewed changes

build: make linker checks more robust

03da4c7

Check for a flag to turn linker warnings into errors. When flags are passed to linkers via the compiler driver using a -Wl,-foo flag, linker warnings may be swallowed rather than bubbling up. Co-authored-by: fanquake <[email protected]>

fanquake force-pushed the make_linker_checks_more_robust branch from d333883 to 03da4c7 Compare May 6, 2020 09:40

laanwj merged commit c6b15ec into bitcoin:master May 6, 2020

fanquake deleted the make_linker_checks_more_robust branch May 6, 2020 23:56

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: make linker checks more robust #17874

build: make linker checks more robust #17874

fanquake commented Jan 5, 2020 •

edited

Loading

DrahtBot commented Jan 5, 2020 •

edited

Loading

DrahtBot commented Jan 6, 2020

laanwj commented Jan 20, 2020

DrahtBot commented Feb 6, 2020

DrahtBot commented Feb 10, 2020

dongcarl commented Mar 9, 2020

hebasto commented Mar 10, 2020

fanquake commented Mar 11, 2020

laanwj Mar 11, 2020

hebasto commented Mar 18, 2020

vasild commented May 6, 2020

fanquake commented May 6, 2020

vasild commented May 6, 2020

build: make linker checks more robust #17874

build: make linker checks more robust #17874

Conversation

fanquake commented Jan 5, 2020 • edited Loading

DrahtBot commented Jan 5, 2020 • edited Loading

Conflicts

DrahtBot commented Jan 6, 2020

Gitian builds

laanwj commented Jan 20, 2020

DrahtBot commented Feb 6, 2020

DrahtBot commented Feb 10, 2020

dongcarl commented Mar 9, 2020

hebasto commented Mar 10, 2020

fanquake commented Mar 11, 2020

Master (b5c7665)

This PR (d333883)

Note

laanwj Mar 11, 2020

Choose a reason for hiding this comment

hebasto commented Mar 18, 2020

vasild commented May 6, 2020

fanquake commented May 6, 2020

vasild commented May 6, 2020

fanquake commented Jan 5, 2020 •

edited

Loading

DrahtBot commented Jan 5, 2020 •

edited

Loading

Master (`b5c7665`)

This PR (`d333883`)