build with -fstack-reuse=none #15983

maflcko · 2019-05-08T18:20:05Z

All versions of gcc that we commonly use for building are subject to bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348. To work around that, set -fstack-reuse=none for all builds. See #15959 (comment)

This option does not exist for clang https://clang.llvm.org/docs/ClangCommandLineReference.html#target-independent-compilation-options, but it does for gcc https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html

gmaxwell · 2019-05-08T20:20:26Z

@jamesob any chance you can benchmark?

jamesob · 2019-05-08T20:21:27Z

@gmaxwell yep, running now.

jamesob · 2019-05-09T00:36:38Z

Performance came back negligibly different, though I'm not totally sure this PR worked for me as intended. I double-checked to see if -fstack-reuse=none made it into the build parameters, and I don't see it in any Makefiles after configuration (gcc (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609):

Wed 8 20:32 james/.bitcoinperf/fad88bd6c9f85e6e7f8fb66a94aa75c67d26b7d8
$ grep -R 'fstack-reuse' .

./autom4te.cache/output.0:{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether C++ compiler accepts -fstack-reuse=none" >&5
./autom4te.cache/output.0:$as_echo_n "checking whether C++ compiler accepts -fstack-reuse=none... " >&6; }
./autom4te.cache/output.0:  CXXFLAGS="$CXXFLAGS  -fstack-reuse=none"
./autom4te.cache/output.0:  HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-reuse=none"
./autom4te.cache/traces.0:m4trace:configure.ac:737: -1- AX_CHECK_COMPILE_FLAG([-fstack-reuse=none], [HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-reuse=none"])

In any case, here's the bench data:

1905-buildStackReuseNone vs. master (absolute)

bench name	x	1905-buildStackReuseNone	master
ibd.local.500005.505000.dbcache=2048	3	304.0416 (± 3.5953)	312.8965 (± 4.2451)
ibd.local.500005.505000.dbcache=2048.mem-usage	3	2463438.6667 (± 3039.5620)	2462478.0000 (± 5874.0000)
build.make.7.gcc	1	302.9981 (± 0.0000)
build.make.7.gcc.mem-usage	1	1024568.0000 (± 0.0000)
ibd.local.500000.505000.dbcache=2048	1	315.0404 (± 0.0000)
ibd.local.500000.505000.dbcache=2048.mem-usage	1	2458576.0000 (± 0.0000)

1905-buildStackReuseNone vs. master (relative)

bench name	x	1905-buildStackReuseNone	master
ibd.local.500005.505000.dbcache=2048	3	1.00	1.029
ibd.local.500005.505000.dbcache=2048.mem-usage	3	1.00	1.000
build.make.7.gcc	1	1.00
build.make.7.gcc.mem-usage	1	1.00
ibd.local.500000.505000.dbcache=2048	1	1.00
ibd.local.500000.505000.dbcache=2048.mem-usage	1	1.00

maflcko · 2019-05-09T12:04:57Z

It should be supported for all versions of gcc we support: https://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Code-Gen-Options.html

I had to run ./autogen.sh && ./configure for it to end up in the CXXFLAGS (they are printed at the end of ./configure)

Also, the results of the micro benchmarks would be useful.

maflcko · 2019-05-09T12:33:28Z

See for example, how it is picked up on trusty: https://travis-ci.org/bitcoin/bitcoin/jobs/529914026#L2522

laanwj · 2019-05-14T18:08:52Z

Tested that build with clang still works, it rejects the argument as expected:

configure:23543: checking whether C++ compiler accepts -fstack-reuse=none
configure:23562: /opt/clang90/bin/clang++ -std=c++11 -c -g -O2  -fstack-reuse=none -ggdb -DLIBEVENT_EXPERIMENTAL -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
clang-9: error: unknown argument: '-fstack-reuse=none'
configure:23562: $? = 1

Checked that the argument is successfully passed for gcc (Ubuntu 7.4.0-1ubuntu1~18.04) 7.4.0

configure:23543: checking whether C++ compiler accepts -fstack-reuse=none
configure:23562: g++ -std=c++11 -c -O2 -g  -fstack-reuse=none  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
configure:23562: $? = 0
configure:23570: result: yes

$ make V=1
Making all in src
...
/bin/bash ../libtool  --tag=CXX --preserve-dup-deps  --mode=compile /usr/bin/ccache g++ -std=c++11 ...-fstack-reuse=none ...`crypto/chacha20.cpp

ryanofsky

utACK fad88bd6c9f85e6e7f8fb66a94aa75c67d26b7d8. Not tested, but config file change looks right and should do what's described.

configure.ac

ryanofsky

utACK fae91e3b19627cba63f05caa18eb990888684036. Just new comment added.

configure.ac

DrahtBot · 2019-05-15T22:59:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15985 (Add test for GCC bug 90348 by sipa)

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.

faf38bc build with -fstack-reuse=none (MarcoFalke) Pull request description: All versions of gcc that we commonly use for building are subject to bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348. To work around that, set `-fstack-reuse=none` for all builds. See #15959 (comment) This option does not exist for clang https://clang.llvm.org/docs/ClangCommandLineReference.html#target-independent-compilation-options, but it does for gcc https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html ACKs for commit faf38b: Tree-SHA512: f5597583402d31ea7b89ac358a6f4a537bbb82a1dde2bd41bac65ee0fd88c7af75ee4c24c6b8eed6b139bf4dbf07ab2987f8fc30fb3eab179cd2d753a8a2a47d

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

58e291c Add test for GCC bug 90348 (Pieter Wuille) Pull request description: This adds a test for GCC bug 90348 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348), using a test case extracted from our own `sha256d64` test in crypto_tests.cpp, which was failing on some platforms. This is based on top of bitcoin#15983 to make sure the bug doesn't trigger (it does in some Travis configurations without it). ACKs for commit 58e291: Tree-SHA512: 4dc9084e92dd143a53930e42bb68e33d922a2a2b891406b259d3a0bed4511dcc49e7447a7a8e4eb793a26e3eacb188ca293b71e0e061f9b3230f8e7fcfd29525

fanquake · 2019-06-07T08:01:14Z

Included for backport in #16035.

maflcko · 2019-06-07T08:01:58Z

@fanquake What about 17.2?

fanquake · 2019-06-07T08:03:58Z

@MarcoFalke I'll do that now so it's not forgotten.

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

fanquake · 2019-06-07T08:31:13Z

Done in #16163.

… case b5a4abe Add test for GCC bug 90348 (Pieter Wuille) 05fb9f7 build with -fstack-reuse=none (MarcoFalke) Pull request description: Backports: * [build with -fstack-reuse=none](#15983) * [Add test for GCC bug 90348](#15985) b5a4abe has been modified to replace the `setup_common.h` with `test_bitcoin.h` include. ACKs for commit b5a4ab: Empact: ACK b5a4abe by review of the linked PRs, the GCC bug and option, and visual inspection/comparison of the ported code laanwj: Code review + relevancy for backport ACK b5a4abe Tree-SHA512: cdfdc6e2f208e8dc6a8a86cd7a7ed0f2a6f96604a0663efc970f580f693c1975353341fa8434b23de3cb681e03c6918e3342178752ed595d16a0ec50db913266

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

faf38bc build with -fstack-reuse=none (MarcoFalke) Pull request description: All versions of gcc that we commonly use for building are subject to bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348. To work around that, set `-fstack-reuse=none` for all builds. See bitcoin#15959 (comment) This option does not exist for clang https://clang.llvm.org/docs/ClangCommandLineReference.html#target-independent-compilation-options, but it does for gcc https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html ACKs for commit faf38b: Tree-SHA512: f5597583402d31ea7b89ac358a6f4a537bbb82a1dde2bd41bac65ee0fd88c7af75ee4c24c6b8eed6b139bf4dbf07ab2987f8fc30fb3eab179cd2d753a8a2a47d

Ref: bitcoin/bitcoin#15983

58e291c Add test for GCC bug 90348 (Pieter Wuille) Pull request description: This adds a test for GCC bug 90348 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348), using a test case extracted from our own `sha256d64` test in crypto_tests.cpp, which was failing on some platforms. This is based on top of bitcoin#15983 to make sure the bug doesn't trigger (it does in some Travis configurations without it). ACKs for commit 58e291: Tree-SHA512: 4dc9084e92dd143a53930e42bb68e33d922a2a2b891406b259d3a0bed4511dcc49e7447a7a8e4eb793a26e3eacb188ca293b71e0e061f9b3230f8e7fcfd29525

maflcko added Build system Needs backport labels May 8, 2019

maflcko added this to the 0.17.2 milestone May 8, 2019

maflcko mentioned this pull request May 8, 2019

refactor: Work around gcc compiler bug 90348 #15959

Closed

sipa mentioned this pull request May 8, 2019

Add test for GCC bug 90348 #15985

Merged

ryanofsky approved these changes May 15, 2019

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

maflcko force-pushed the 1905-buildStackReuseNone branch from fad88bd to fae91e3 Compare May 15, 2019 19:15

ryanofsky approved these changes May 15, 2019

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

build with -fstack-reuse=none

faf38bc

maflcko force-pushed the 1905-buildStackReuseNone branch from fae91e3 to faf38bc Compare May 15, 2019 19:41

maflcko merged commit faf38bc into bitcoin:master May 16, 2019

maflcko deleted the 1905-buildStackReuseNone branch May 16, 2019 14:07

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request May 16, 2019

build with -fstack-reuse=none

5935f01

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

practicalswift mentioned this pull request May 26, 2019

32-bit test failure: test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed #14580

Closed

fanquake removed the Needs backport label Jun 7, 2019

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 7, 2019

build with -fstack-reuse=none

05fb9f7

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

fanquake mentioned this pull request Jun 7, 2019

0.17.2 backport: build with -fstack-reuse=none and test case #16163

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019

build with -fstack-reuse=none

69f7db3

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019

build with -fstack-reuse=none

9f1f039

Github-Pull: bitcoin#15983 Rebased-From: faf38bc

barton2526 added a commit to barton2526/Gridcoin-Research that referenced this pull request Jul 13, 2021

build with -fstack-reuse=none

c06743b

Ref: bitcoin/bitcoin#15983

barton2526 mentioned this pull request Jul 13, 2021

build: Use -fstack-reuse=none gridcoin-community/Gridcoin-Research#2232

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

build with -fstack-reuse=none #15983

build with -fstack-reuse=none #15983

Conversation

maflcko commented May 8, 2019

Uh oh!

gmaxwell commented May 8, 2019

Uh oh!

jamesob commented May 8, 2019

Uh oh!

jamesob commented May 9, 2019

1905-buildStackReuseNone vs. master (absolute)

1905-buildStackReuseNone vs. master (relative)

Uh oh!

maflcko commented May 9, 2019

Uh oh!

maflcko commented May 9, 2019

Uh oh!

laanwj commented May 14, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented May 15, 2019

Conflicts

Uh oh!

fanquake commented Jun 7, 2019

Uh oh!

maflcko commented Jun 7, 2019

Uh oh!

fanquake commented Jun 7, 2019

Uh oh!

fanquake commented Jun 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants