Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,14 @@ if( CLANG_HAVE_DLFCN_H )
cmake_pop_check_state()
endif()

include(CheckCXXSourceCompiles)
check_cxx_source_compiles("
extern \"C\" void *__emutls_get_address(void *);
int main() {
return __emutls_get_address((void *)0) != (void *)0;
}
" CLANG_HAVE_EMUTLS_GET_ADDRESS)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@compnerd ping


set(CLANG_RESOURCE_DIR "" CACHE STRING
"Relative directory from the Clang binary to its resource files.")

Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Config/config.h.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,7 @@
/* Enable the experimental new constant interpreter by default */
#cmakedefine01 CLANG_USE_EXPERIMENTAL_CONST_INTERP

/* Define if __emutls_get_address is available in the compiler runtime */
#cmakedefine01 CLANG_HAVE_EMUTLS_GET_ADDRESS

#endif
12 changes: 5 additions & 7 deletions clang/lib/Interpreter/IncrementalExecutor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#endif // __EMSCRIPTEN__

#include "clang/Basic/TargetInfo.h"
#include "clang/Config/config.h"
#include "clang/Driver/Compilation.h"
#include "clang/Driver/Driver.h"
#include "clang/Driver/ToolChain.h"
Expand Down Expand Up @@ -70,13 +71,10 @@
// in a static archive and nothing else references it, it is never linked in and
// ORC's process-symbol lookup cannot resolve it. Referencing it here
// force-links the archive member so it is present regardless of how the host
// provides it. Excluded where an emulated-TLS runtime is not guaranteed on the
// link line, so the reference would fail to link: non-Unix (MSVC has no such
// runtime), Emscripten (the wasm executor below does not use this JIT path),
// and AIX / z/OS (whose runtimes may not provide the symbol). On those hosts
// thread_locals instead rely on process-symbol lookup, unchanged from before.
#if defined(LLVM_ON_UNIX) && !defined(__EMSCRIPTEN__) && !defined(_AIX) && \
!defined(__MVS__) && !defined(__FreeBSD__)
// provides it. Defined if available at configure time
// (CLANG_HAVE_EMUTLS_GET_ADDRESS). When unavailable, thread_locals instead rely
// on process-symbol lookup.
#if CLANG_HAVE_EMUTLS_GET_ADDRESS
extern "C" void *__emutls_get_address(void *);
static void *getEmuTLSGetAddressPtr() {
return reinterpret_cast<void *>(&__emutls_get_address);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ write_cmake_config("Config") {
"CLANG_SYSTEMZ_DEFAULT_ARCH=z10",
"PPC_LINUX_DEFAULT_IEEELONGDOUBLE=",
"CLANG_USE_EXPERIMENTAL_CONST_INTERP=",
"CLANG_HAVE_EMUTLS_GET_ADDRESS=",
]

if (clang_enable_static_analyzer) {
Expand Down
1 change: 1 addition & 0 deletions utils/bazel/llvm-project-overlay/clang/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -1948,6 +1948,7 @@ cc_library(
":ast",
":basic",
":codegen",
":config",
":driver",
":edit",
":frontend",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@
/* Enable the experimental new constant interpreter by default */
#define CLANG_USE_EXPERIMENTAL_CONST_INTERP 0

/* Define if __emutls_get_address is available in the compiler runtime */
#define CLANG_HAVE_EMUTLS_GET_ADDRESS 0

/* Directly provide definitions here behind platform preprocessor definitions.
* The preprocessor conditions are sufficient to handle all of the configuration
* on platforms targeted by Bazel, and defining these here more faithfully
Expand Down
Loading