Page MenuHomePhabricator

libclang.so: Make SONAME independent from LLVM version
ClosedPublic

Authored by tstellar on Jul 6 2021, 9:20 PM.

Diff Detail

Event Timeline

tstellar created this revision.Jul 6 2021, 9:20 PM
tstellar requested review of this revision.Jul 6 2021, 9:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 9:20 PM

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.

MaskRay added inline comments.Jul 6 2021, 11:49 PM
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?

tstellar updated this revision to Diff 360517.Jul 21 2021, 10:43 AM
tstellar marked 3 inline comments as done.

Address some review comments.

clang/tools/libclang/CMakeLists.txt
82

Are you saying we should generate the version script using llvm-readelf?

MaskRay added inline comments.Jul 21 2021, 10:50 AM
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

MaskRay added inline comments.Jul 21 2021, 10:52 AM
clang/tools/libclang/CMakeLists.txt
82

Oh, you used local: *. Then there may need a test that there is no localized clang_* symbol.

tstellar updated this revision to Diff 360654.Jul 21 2021, 5:08 PM

Added a test case.

tstellar updated this revision to Diff 360656.Jul 21 2021, 5:09 PM

Add missing file.

MaskRay added inline comments.Jul 24 2021, 2:39 PM
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: *;
}
MaskRay requested changes to this revision.Jul 24 2021, 2:39 PM
This revision now requires changes to proceed.Jul 24 2021, 2:39 PM
tstellar updated this revision to Diff 361734.Jul 26 2021, 11:18 AM

Add test case for local symbols and reformat version script.

tstellar marked 3 inline comments as done.Jul 26 2021, 11:20 AM
MaskRay accepted this revision.Jul 26 2021, 11:24 AM

Looks great!

This revision is now accepted and ready to land.Jul 26 2021, 11:24 AM
This revision was landed with ongoing or failed builds.Jul 26 2021, 4:37 PM
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
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?

tstellar added inline comments.Jul 26 2021, 6:59 PM
clang/tools/libclang/CMakeLists.txt
67

This was a mistake. This has been changed in trunk now to generate into CMAKE_CURRENT_BINARY_DIR.

thakis added inline comments.Jul 26 2021, 7:03 PM
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.

thakis added inline comments.Jul 26 2021, 7:12 PM
clang/test/LibClang/symbols.test
2

Aha, libclang.so doesn't exist then and then the config.pipefail=False makes things pass then. Subtle.

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.

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.

jsji added a subscriber: jsji.Jul 27 2021, 12:19 PM
jsji added inline comments.
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.

jsji added inline comments.Jul 27 2021, 1:16 PM
clang/tools/libclang/CMakeLists.txt
79

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')

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?

I fixed all these issues in the main branch yesterday.

clang/test/LibClang/symbols.test
2

Aha, libclang.so doesn't exist then and then the config.pipefail=False makes things pass then. Subtle.

This was not my intention, but I'm glad it works.

jrtc27 added a subscriber: jrtc27.EditedJul 28 2021, 8:02 AM

I'm seeing the test fail locally (on ca0fe3447fb85762838468537d93d4ef82c5a1af) with:

00000000000d1799 t clang_CompileCommand_getNumMappedSources

being found by grep.

I'm seeing the test fail locally (on ca0fe3447fb85762838468537d93d4ef82c5a1af) with:

00000000000d1799 t clang_CompileCommand_getNumMappedSources

being found by grep.

What Operating System?

I'm seeing the test fail locally (on ca0fe3447fb85762838468537d93d4ef82c5a1af) with:

00000000000d1799 t clang_CompileCommand_getNumMappedSources

being found by grep.

What Operating System?

Ubuntu 18.04, amd64, using -fuse-ld=gold

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.

Can you let me know if D106974 fixes the problem?

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 :)