Skip to content

Conversation

@theuni
Copy link
Member

@theuni theuni commented Nov 10, 2015

Rather than setting -fPIC wholesale, only set it for the qt objects.

The first commit restructures to move the fPIE flag out, the second is the actual change.

Note that -fPIC is used for testing qt until we get to the actual fPIE test. That's to prevent us from bombing out early due to qt's header yelling about PIE before we actually want to test it.

This allows for fPIE to be used selectively.
But only if qt was built with reduced relocations.
@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

Great work, utACK (will test later)

@maflcko
Copy link
Member

maflcko commented Nov 10, 2015

Thanks, Concept ACK.

Can confirm default settings ($ ./autogen.sh ;./configure ;make) work again with 69d0513 on latest fedora. (You may want to put "closes #6248" in the OP)

@jonasschnelli
Copy link
Contributor

@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

On Ubuntu 15.10 this gives:

checking whether -fPIE can be used with this Qt config... no

Which is correct. However on On Ubuntu 14.04 I also get:

checking whether -fPIE can be used with this Qt config... no

While I know in fact that that does work. Error in config.log is:

configure:23385: g++ -c -fPIE -g -O2 -Wall -Wextra -Wformat -Wformat-security -Wno-unused-parameter -Wno-self-assign -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtWidgets    -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
conftest.cpp: In function 'int main()':
conftest.cpp:67:15: error: 'choke' was not declared in this scope
               choke;
               ^
At global scope:

Strange. QT_REDUCE_RELOCATIONS is indeed enabled:

./qt5/QtCore/qconfig.h:#define QT_REDUCE_RELOCATIONS

However it just works with fPIE. Gcc version issue?

@theuni
Copy link
Member Author

theuni commented Nov 10, 2015

@laanwj I believe it's a Qt issue. They tightened the requirements with newer qt versions, -fPIE used to compile fine. But -fPIE + gcc5 + older Qt will still likely cause problems regardless if it does compile.

So the real issue is -fPIE + newer gcc regardless of when qt tightened the error. I think it makes sense to always fall back to -fPIC any time that reduce_relocations was used. The alternative would be a blacklist/whitelist for compilers, which i was trying to avoid.

@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

Wouldn't compiling+linking an example program w/ -fPIE and Qt be the most sure way to check? Or is there a specific reason you didn't go that route?

@theuni
Copy link
Member Author

theuni commented Nov 10, 2015

@laanwj It compiles/links fine with the busted config. So there's no point in linking if we're not going to run it, and a run-test is a no-go because it doesn't work with cross.

@theuni
Copy link
Member Author

theuni commented Nov 10, 2015

The alternative would be compiling a stub object with fPIE and objdump to check for relocations.

@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

Huh are we talking about the same issue?
On at least ubuntu 15.10 this fPIE/freduced-relocations conflict causes a build-time issue, not a run-time isue.

@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

This is the error I get (one for every executable that uses qt):

In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qnamespace.h:37:0,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs.h:41,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qobject.h:40,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QObject:1,
                 from qt/test/uritests.h:8,
                 from qt/test/test_main.cpp:10:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qglobal.h:1052:4: error: #error "You must build your code with position independent code if Qt was built with -reduce-relocations. " "Compile your code with -fPIC (-fPIE is not enough)."
 #  error "You must build your code with position independent code if Qt was built with -reduce-relocations. "\
    ^

@theuni
Copy link
Member Author

theuni commented Nov 10, 2015

@laanwj Sorry for being unclear. To summarize the issue:

Qt build can enable -Bsymbolic-functions via the reduce-relocations config option. The qt devs assumed that -fPIC or -fPIE would disable copy-relocations, but gcc5 changed that behavior and allows copy-relocs with -fPIE. So -fPIC is the only way to guarantee that -Bsymbolic-functions will work as intended.

Source: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65886#c16

When Qt discovered this issue (somewhere around 5.4), they added the check in qconfig.h so that compilation would fail in this QT_REDUCE_RELOCATIONS+-fPIE case. Prior to that, the check looked like this:

#if !defined(QT_BOOTSTRAPPED) && defined(QT_REDUCE_RELOCATIONS) && defined(__ELF__) && !defined(__PIC__) && !defined(__PIE__)
#  error "You must build your code with position independent code if Qt was built with -reduce-relocations. "\
         "Compile your code with -fPIC or -fPIE."
#endif

Just because compiling succeeds in versions with the old check, that does not mean that it's safe to build/run with gcc5+. So testing to see if something builds against qt+-fPIE is not enough, as older versions weren't aware of the problem.

@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

Ok, fair enough

@laanwj laanwj merged commit 69d0513 into bitcoin:master Nov 11, 2015
laanwj added a commit that referenced this pull request Nov 11, 2015
69d0513 build: Use fPIC rather than fPIE for qt objects. (Cory Fields)
17c4d9d build: Split hardening/fPIE options out (Cory Fields)
@laanwj
Copy link
Member

laanwj commented Nov 11, 2015

Tested ACK.

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Nov 18, 2015
This allows for fPIE to be used selectively.

Github-Pull: bitcoin#6978
Rebased-From: 17c4d9d
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Nov 18, 2015
But only if qt was built with reduced relocations.

Github-Pull: bitcoin#6978
Rebased-From: 69d0513
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Nov 27, 2015
This allows for fPIE to be used selectively.

Github-Pull: bitcoin#6978
Rebased-From: 17c4d9d
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 2015
This allows for fPIE to be used selectively.

Github-Pull: bitcoin#6978
Rebased-From: 17c4d9d
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 2015
But only if qt was built with reduced relocations.

Github-Pull: bitcoin#6978
Rebased-From: 69d0513
@anontahoe
Copy link

pardon me for necroing this thread but if -fPIC weakens PaX/Grsec patches then i wonder if silently using it as a fallback option is the right way to solve this.
maybe a clear warning can be shown and more effort undertaken to convince upstream or package maintainers to ship Qt without reduce-relocations?

zkbot added a commit to zcash/zcash that referenced this pull request Dec 1, 2017
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 15, 2017
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
kotodev pushed a commit to koto-dev/koto.old that referenced this pull request Jan 25, 2018
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
renium9 added a commit to koto-dev/koto.old that referenced this pull request Feb 6, 2018
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

# Conflicts:
#	configure.ac
#	src/Makefile.am
#	src/Makefile.gtest.include
#	src/Makefile.test.include
#	zcutil/build.sh
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants