-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fix g++-10 link issue when building with -std=c++20. #1103
Conversation
…ssue Within `libraries/wasm-jit/Source`, `WASM` depends on `IR` and `Logging`, and `IR` depends on `Logging`. With the current declaration of `WASM::check_limits` in `WASM`, we have the following g++-10 link error when building with `-std=c++20`: ``` _64-linux-gnu/libz.so -ldl libraries/libfc/secp256k1/libsecp256k1.a libraries/chainbase/libchainbase.a -lpthread libraries/softfloat/libsoftfloat.a && : /usr/bin/ld: libraries/wasm-jit/Source/IR/libIR.a(DisassemblyNames.cpp.o): warning: relocation against `_ZN4WASM12check_limitsE' in read-only section `.text' /usr/bin/ld: libraries/wasm-jit/Source/IR/libIR.a(DisassemblyNames.cpp.o): in function `void Serialization::serialize<Serialization::MemoryInputStream>(Serialization::MemoryInputStream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)': /home/greg/github/enf/leap/libraries/wasm-jit/Include/Inline/Serialization.h:274: undefined reference to `WASM::check_limits' /usr/bin/ld: warning: creating DT_TEXTREL in a PIE collect2: error: ld returned 1 exit status ``` Moving the declaration of `WASM::check_limits` in `Logging` (end leaf of dependency graph) fixes the issue.
namespace WASM | ||
{ | ||
bool check_limits = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider using an inline static
of a struct instead? Seems like that would be cleaner than extern
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do the minimal change to fix the issue. In my first attempt I had changed it to a function accessing a static variable, but there were more changes and I tried to find the simplest fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like the idea of an inline static
struct member, as it relies on a C++ feature that I feel should be more foolproof. Still since it is not my code, and this is a workaround for a specific compiler issue, I'd prefer to keep the change as minimal as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a little strange putting something that has nothing to do with logging in the Logging
library. Could it work in IR
instead? The Logging library could easily be on the chopping block whenever someone has time to keep trimming down WAVM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Matt, it looks like it is working fine with the variable in IR
. I'll push that change.
Within
libraries/wasm-jit/Source
,WASM
depends onIR
andLogging
, andIR
depends onLogging
.With the current declaration of
WASM::check_limits
inWASM
, we have the following g++-10 link error when building with-std=c++20
:Moving the declaration of
WASM::check_limits
inLogging
(end leaf of dependency graph) fixes the issue.