Fix getauxval calls in randomenv.cpp #20594
Conversation
6b6a5d6 to
836a3dc
Compare
|
Code review ACK 836a3dc |
|
utACK 836a3dc |
|
#20358 is marked for backport, so should this be marked as well or neither? |
|
I don't think this qualifies a backport. |
|
I don't think we're going to backport #20594, so this one shouldn't be needed to be backported either. |
Gitian builds
|
| [ AC_MSG_RESULT(no); HAVE_STRONG_GETAUXVAL=0 ] | ||
| ) | ||
|
|
||
| AC_MSG_CHECKING(for weak getauxval support in the compiler) | ||
| AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ | ||
| #ifdef __linux__ | ||
| unsigned long getauxval(unsigned long type) __attribute__((weak)); | ||
| #define AT_HWCAP 16 |
There was a problem hiding this comment.
This seems very fragile. On what target is getauxval only weak?
There was a problem hiding this comment.
Why would it be (more) fragile? Weak linking is already pretty fragile AFAIK.
IMO this weak linking passes always. See masters Win64 build as an example.
AFAIK this check is part of googles crc32c suite.
Limiting it to linux makes absolute sense to me since getauxval (it doesn't exists on Mac/win).
There was a problem hiding this comment.
Sorry, I do mean the weak linking entirely. What do we lose, if we only check for strong linking?
There was a problem hiding this comment.
That's a valid question.
AFAIK the google crc32c getauxval weak linking check is for android (< API level 20).
Crc32c is an upstream upstream subtree. I guess if we don't want to check for the weak linking, we might want to PR that there.
There was a problem hiding this comment.
I assume if we just don't define it, that code block simply won't be used?
There was a problem hiding this comment.
This seems very fragile. On what target is getauxval only weak?
a) getauxval only exists on Linux (OSes with the Linux kernel, I mean)
b) by nature of weak linking, it could weakly link on every platform, this makes no sense though
|
I think this fix is fine. Removing the weak check completely is something that could be considered, but might interfere with Android. It doesn't need to be decided in this PR. |
836a3dc Avoid weak-linked getauxval support on non-linux platforms (like macOS) (Jonas Schnelli) 41a413b Define correct symbols for getauxval (Jonas Schnelli) Pull request description: PR bitcoin#20358 made use of the two preprocessor symbols `HAVE_STRONG_GETAUXVAL` as well as `HAVE_WEAK_GETAUXVAL`. These symbols have not been defined in configure.ac. They where only passed selective as CRC32 CPPFLAGS in https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include#L16. PR bitcoin#20358 would have broken the macOS build since `getauxval` is not supported on macOS (but weak-linking does pass). This PR defines the two symbols correctly and reduces calls to `getauxval` to linux. ACKs for top commit: laanwj: Code review ACK 836a3dc jonatack: utACK 836a3dc Tree-SHA512: 6527f4a617b937f4c368a3cb1c162f1ac38a6f5e6341295554961eaf322906e9b27398a6f7b00819854ceebb5c828d3e6ce0a779edd769adc4053ce8beda3739
836a3dc Avoid weak-linked getauxval support on non-linux platforms (like macOS) (Jonas Schnelli) 41a413b Define correct symbols for getauxval (Jonas Schnelli) Pull request description: PR bitcoin#20358 made use of the two preprocessor symbols `HAVE_STRONG_GETAUXVAL` as well as `HAVE_WEAK_GETAUXVAL`. These symbols have not been defined in configure.ac. They where only passed selective as CRC32 CPPFLAGS in https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include#L16. PR bitcoin#20358 would have broken the macOS build since `getauxval` is not supported on macOS (but weak-linking does pass). This PR defines the two symbols correctly and reduces calls to `getauxval` to linux. ACKs for top commit: laanwj: Code review ACK 836a3dc jonatack: utACK 836a3dc Tree-SHA512: 6527f4a617b937f4c368a3cb1c162f1ac38a6f5e6341295554961eaf322906e9b27398a6f7b00819854ceebb5c828d3e6ce0a779edd769adc4053ce8beda3739
836a3dc Avoid weak-linked getauxval support on non-linux platforms (like macOS) (Jonas Schnelli) 41a413b Define correct symbols for getauxval (Jonas Schnelli) Pull request description: PR bitcoin#20358 made use of the two preprocessor symbols `HAVE_STRONG_GETAUXVAL` as well as `HAVE_WEAK_GETAUXVAL`. These symbols have not been defined in configure.ac. They where only passed selective as CRC32 CPPFLAGS in https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include#L16. PR bitcoin#20358 would have broken the macOS build since `getauxval` is not supported on macOS (but weak-linking does pass). This PR defines the two symbols correctly and reduces calls to `getauxval` to linux. ACKs for top commit: laanwj: Code review ACK 836a3dc jonatack: utACK 836a3dc Tree-SHA512: 6527f4a617b937f4c368a3cb1c162f1ac38a6f5e6341295554961eaf322906e9b27398a6f7b00819854ceebb5c828d3e6ce0a779edd769adc4053ce8beda3739
PR #20358 made use of the two preprocessor symbols
HAVE_STRONG_GETAUXVALas well asHAVE_WEAK_GETAUXVAL.These symbols have not been defined in configure.ac. They where only passed selective as CRC32 CPPFLAGS in https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include#L16.
PR #20358 would have broken the macOS build since
getauxvalis not supported on macOS (but weak-linking does pass).This PR defines the two symbols correctly and reduces calls to
getauxvalto linux.