[clang-repl] Add CLANG_ENABLE_EMUTLS_GET_ADDRESS cmake option - #217700
[clang-repl] Add CLANG_ENABLE_EMUTLS_GET_ADDRESS cmake option#217700pranavk wants to merge 1 commit into
Conversation
llvmorg-github-actions
Bot
added
clang:frontend
| option(CLANG_USE_EXPERIMENTAL_CONST_INTERP | ||
| "Use the new experimental constant interpreter for compile-time evaluation." OFF) | ||
|
|
||
| option(CLANG_ENABLE_EMUTLS_GET_ADDRESS |
There was a problem hiding this comment.
I was wondering if we can detect that during configure time instead of having a separate option?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
looking at windows PR checks, for reasons i don't understand, the cmake check passes on Windows but then fails to find __emutls_get_address during link time. any ideas? |
Detect if __emutls_get_address is available in the compiler runtime at configure time via check_cxx_source_compiles (CLANG_HAVE_EMUTLS_GET_ADDRESS) and guard its extern "C" declaration and reference in IncrementalExecutor.cpp. This prevents link errors in environments/runtimes where __emutls_get_address is not available. TAG=agy CONV=dd895981-3d14-4e98-a942-6aedc8fbfd77
|
I changed the cmake check a bit to call the function. i don't have a windows machine but maybe it was getting optimized away? i will wait for the PR CI checks. |
| int main() { | ||
| return __emutls_get_address((void *)0) != (void *)0; | ||
| } | ||
| " CLANG_HAVE_EMUTLS_GET_ADDRESS) |
There was a problem hiding this comment.
I do not understand this check. This basically attempts to determine it by means of a magically linked library rather than some condition. We should simply hoist the CPP condition into CMake and require an alternative answer to be explicit opt-in.
There was a problem hiding this comment.
For context, this is trying to fix #209717 which requires __emutls_get_address to be available at link time.
This PR is trying to move all that logic into cmake configure time.
There was another version of this PR that didn't have this check and just required explicitly setting -DCLANG_HAVE_EMUTLS_GET_ADDRESS= but @vgvassilev wanted to see if we could get that auto set during configure time
There was a problem hiding this comment.
The windows checks are now passing with the new version of this cmake check. Too bad I didn't preserve the commit history. The earlier check looked like this:
extern "C" void *__emutls_get_address(void *);
int main() {
void *p = (void *)&__emutls_get_address;
return p == (void *)0;
}
So it seems like during the PR check on Windows, it was getting optimized away and we don't have any unresolved symbol in the end. Changing the check to call the function forces the linker to look for the definition at link time which is what the original PR is doing as well.
There was a problem hiding this comment.
@compnerd let me know if there's anything I can do here. It's currently blocking our internal processes. So I would like to get this fixed sooner.
The current check takes care of @vgvassilev concerns without forcing downstream platforms to provide __emutls_get_address during link time.
I don't see the logs because they have been removed by the new run. What is "Windows" here (i.e. what environment)? MinGW? MSVC? Cygwin? Something else? At least on MSVC, it shouldn't do that as weak resolution is not supported. MinGW does have a custom mechanism for weak symbols IIRC. |
It's MSVC afaics that we are using in PR checkers. |
|
Windows CI is an MSVC environment, minus the fact we build with |
This is needed to fix #209717. llvm/llvm-project#217700 tries to fix it cleanly but it's blocked on review. Get this in while we wait for review.
5 participants
Guard the extern "C" declaration and reference to __emutls_get_address in IncrementalExecutor.cpp behind a new CMake option CLANG_ENABLE_EMUTLS_GET_ADDRESS, which is OFF by default.
This prevents link errors in environments/runtimes where __emutls_get_address is not available.
Replace all other guards with the new cmake option.