Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I tried to keep this simple and only add the Linux support for now. There seem to be some subtle differences of how different operating systems handle export lists and linker scripts, and I wasn't confident about how to make this work on those systems.
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 82 | What does this do? A hard-coded list cannot catch up with the real dynamic symbol list. We'll have to parse llvm-readelf --dyn-syms output... | |
| clang/tools/libclang/libclang.map | ||
| 406 | Make its indentation match global: | |
| 414 | The dependency syntax is completely ignored by LLD and GNU ld. Having it can only cause confusion. | |
| clang/tools/libclang/linker-script-to-export-list.py | ||
| 11 | syntax error? | |
Address some review comments.
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 82 | Are you saying we should generate the version script using llvm-readelf? | |
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 82 | We may need a test checking that if --version-script is used, there is no clang_* symbol which isn't versioned. This can be checked by running llvm-nm -Dj --defined-only on libclang.so | |
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 82 | Oh, you used local: *. Then there may need a test that there is no localized clang_* symbol. | |
| clang/test/LibClang/lit.local.cfg | ||
|---|---|---|
| 2 | delete blank line | |
| clang/test/LibClang/symbols.test | ||
| 3 | Also a test that no local symbol is called clang_* A local: * can easily localize unintented clang_* symbols | |
| clang/tools/libclang/libclang.map | ||
| 406 | You may consider LLVM_13 {
/* no global: here */
clang_foobar;
}
local {
local: *;
} | |
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 67 | This is still set to CURRENT_SOURCE_DIR. Is this set anywhere else, or does the action below create a file in the source dir now? | |
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 67 | This was a mistake. This has been changed in trunk now to generate into CMAKE_CURRENT_BINARY_DIR. | |
| clang/test/LibClang/symbols.test | ||
|---|---|---|
| 2 | I think it's possible to configure the build to only build a static libclang. This should probably only be done if libclang is built dynamically? | |
For future reference this was very difficult to merge into external changes. It looks like you resorted this at the same time as renaming it, and that messes up git's rename logic.
| clang/test/LibClang/symbols.test | ||
|---|---|---|
| 2 | Aha, libclang.so doesn't exist then and then the config.pipefail=False makes things pass then. Subtle. | |
OK, sorry about that. Is there some way to check this before committing? It seems like it might be useful to have a pre-commit test for it.
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 79 | I think we should check LLVM_HAVE_LINK_VERSION_SCRIPT here, there are platforms that does NOT support VERSION_SCRIPT. | |
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 79 | https://reviews.llvm.org/D106914 posted to fix it. | |
Also, this python script just doesn't work. It's missing a sys import, a "w" flag, and a new line after each write. It also dumps the output into the source directory. Was this actually tested?
Here's a version that actually works (python 3, not sure if it's valid in 2), although I would much prefer we not write to the source directory during a build.
import re
import os
import sys
input_file = open(sys.argv[1])
with open(sys.argv[2], "w") as output_file:
for line in input_file:
m = re.search('clang_[^;]+', line)
if m:
output_file.write(m.group(0) + '\n')I fixed all these issues in the main branch yesterday.
| clang/test/LibClang/symbols.test | ||
|---|---|---|
| 2 |
This was not my intention, but I'm glad it works. | |
I'm seeing the test fail locally (on ca0fe3447fb85762838468537d93d4ef82c5a1af) with:
00000000000d1799 t clang_CompileCommand_getNumMappedSources
being found by grep.
It looks like that function is missing from libclang.map, I think if you add it there it should fix the test. I'm going to check to see if there are any more missing functions and then commit a fix.
As this is a big deal for packager, maybe it should be part of the release notes ? (maybe it is and I just missed it :)
delete blank line