This enables -emit-ifso to generate an interface library for each .o file. Currently it just writes a text file with the mangled names in it.
Details
- Reviewers
compnerd alexander-shaposhnikov phosek jakehehrlich • espindola rupprecht - Commits
- rG68f29dac4be8: [clang-ifs] Clang Interface Stubs, first version (second landing attempt).
rL363948: [clang-ifs] Clang Interface Stubs, first version (second landing attempt).
rC363948: [clang-ifs] Clang Interface Stubs, first version (second landing attempt).
rG8df7f1a218f2: [clang-ifs] Clang Interface Stubs, first version.
rL363626: [clang-ifs] Clang Interface Stubs, first version.
rC363626: [clang-ifs] Clang Interface Stubs, first version.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Frontend/FrontendActions.cpp | ||
---|---|---|
182 ↗ | (On Diff #198157) | I'm not sure I like this very much. You are templating over a type a function that needs to walk the hierarchy for NamedDecl. This is going to re-emit the function and without LTO cause a large .text contribution for something that should really just be a jump table. Would it not be possible to use a covered switch on the kindof() in the NamedDecl. The covering should ensure that future changes would allow us to find this site. |
189 ↗ | (On Diff #198157) | I understand this, but, it may be nice to have a comment explaining why we only consider default visibility (hidden visibility interfaces cannot be seen by consumers, and protected mode visibility in ELF does not participate in linking, only loading). |
197 ↗ | (On Diff #198157) | Newline before this would be nice. |
199 ↗ | (On Diff #198157) | This is confusing, can you rewrite this as, dropping L204-205: uint8_t Type = isa<VarDecl>(ND) ? llvm::ELF::STT_OBJECT : llvm::ELF::STT_FUNC; |
203 ↗ | (On Diff #198157) | Again, I think that this is slightly confusing, and would prefer a ternary. |
207 ↗ | (On Diff #198157) | Can you not use a universal initialiser? In the case you cannot, std::make_pair would be nicer to use for the type deduction. |
214 ↗ | (On Diff #198157) | debugging left over? |
219 ↗ | (On Diff #198157) | Similar to the previous case, I am slightly worried about the size contributions here. Why not a covered switch? |
222 ↗ | (On Diff #198157) | const auto *D? This also makes very little sense to the casual reader. This cast is very confusing. There is no reason to assume that an arbitrary NamedDecl is a DeclContext. It is only made even more confusing by the fact that the decl that is retrieved is a pointer - since that is an implementation detail of the decl_iterator in the decl_range returned by decls which is implicitly dereferenced by the range based for loop. Making the type explicit by the means of the covered switch and an explicit cast to the DeclContext should help in clearing that up. |
223 ↗ | (On Diff #198157) | Hmm, do we have a guarantee that the decl is named? Could you not hit a _Static_assert or static_assert in this traversal? Or if it is a C++ context, an extern "C" block? What about #pragma comment(...)? Please add test cases for these. |
227 ↗ | (On Diff #198157) | Again, the templating here worries me. |
232 ↗ | (On Diff #198157) | Hmm, what guarantee do we have that the templated type is a template type? |
254 ↗ | (On Diff #198157) | Please put this in an LLVM_DEBUG. |
269 ↗ | (On Diff #198157) | const? I think that this might be easier to parse for the reader as: if (const auto *FD = dyn_cast<FunctionDecl>(ND)) if (FD->isLateTemplateParsed()) return LateParsedDecls.insert(FD).first != LateParsedDecls.end(); else return NamedDecls.insert(FD).first != NamedDecls.end(); if (const auto *VD = dyn_cast<ValueDecl>(ND)) return ValueDecls.insert(VD).first != ValueDecls.end(); return NamedDecls.insert(ND) != NamedDecls.end(); |
292 ↗ | (On Diff #198157) | S is the traditional initialism. |
297 ↗ | (On Diff #198157) | Typo of ||? The field is boolean not a mask. |
341 ↗ | (On Diff #198157) | const auto &E? I wish that we had structured decomposition already. |
clang/include/clang/Driver/Options.td | ||
---|---|---|
626 | Oh, I was specifying experimental in the -interface-stubs-version=experimental-ifo-elf-v1. Do you want the main flag to also be -emit-interface-stubs-experimental?? | |
clang/include/clang/Driver/Types.def | ||
91 | I went with ifs because ifo implies the analog of a .o file and ifso implies the analog of a .so file. I want the tbe/ifs text files to just be thought of as something a little different. Like symbol listings that another tool can assemble into whatever format. | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
1649 | I think that makes sense. | |
clang/lib/Frontend/FrontendActions.cpp | ||
179 ↗ | (On Diff #198157) | Can we do data-structure improvements post landing the first version? |
214 ↗ | (On Diff #198157) | Ah yeah I need to replace this with an llvm_unreachable or something. |
297 ↗ | (On Diff #198157) | It is a bitmask. |
clang/include/clang/Driver/Options.td | ||
---|---|---|
626 | Okay, I can live with that. I guess that was just unclear to me. Doing that is nicer in that there is less churn in the actual flags. | |
clang/include/clang/Driver/Types.def | ||
91 | Oh, I see. Yeah .... torn on that tbh. There are aspects of it as being analogs to .o or .obj as well. But, having them be confused for one another is worse. If this makes sense to you and @rupprecht, WFM. | |
clang/lib/Frontend/FrontendActions.cpp | ||
297 ↗ | (On Diff #198157) | Oh! |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3590 | Couldn't I do this in the cc1 path? |
clang/lib/Frontend/FrontendActions.cpp | ||
---|---|---|
223 ↗ | (On Diff #198157) | HandleNamedDecl bails when the NamedDecl is not a NamedDecl. |
llvm/lib/TextAPI/ELF/TBEHandler.cpp | ||
---|---|---|
137 ↗ | (On Diff #198977) | @jakehehrlich Here, I've added an Endian field to tbe. I think I can just generate the tbe that llvm-elfabi consumes. The experimental yaml output is more for debugging. Tell me what you think. |
@jakehehrlich What else are you looking for here? Can the patch for merging tbe files come later? @compnerd @rupprecht thoughts? I think this is a reasonable compromise: a minimal subset of what the yaml2obj and llvm-elfabi (with the addition of the Endian field) tools can already consume.
This shouldn't emit the .tbe format at all, the .tbe format is meant to be in near 1-1 correspondence with .so files, not with .o files. It has things like DT_NEEDED, and DT_SONAME related information that don't make sense for .o files for instance.
If we put things under an --experimental flags, minimize what we add in the first patch, and try and make each additional field added need based, I'm ok with whatever being added to Clang assuming it doesn't interfere with other parts of the compiler.
llvm/include/llvm/TextAPI/ELF/ELFStub.h | ||
---|---|---|
58 ↗ | (On Diff #198977) | Why do we need or want this? If it is needed can it be added later when needed? Also why is this a string? |
llvm/include/llvm/TextAPI/ELF/ELFStub.h | ||
---|---|---|
58 ↗ | (On Diff #198977) | Just a suggestion, I saw that ELF needs endianness specified and I also saw that the llvm-elfabi tool (the latest patch set from phab anyways) has cmd clangs for specifying endianness and thought it could be nice to specify in the file. Also you are right, this probably shouldn't be a string. I can drop it from this change. |
That sounds good. I wasn't sure if you wanted straight up tbe or not. I can basically keep what we have here and make the heading line include an "experimental" tag so that it cant be directly read by the llvm-elfabi tool without some changes. I do think it should be sufficient to have some subset of tbe as a merging format in llvm-elbabi. Will post an update.
Changing !tapi-tbe to !experimental-tapi-elf-v1, removing Endianness field from tapi-tbe.
So my latest diff addresses this. The schema resembles the tbe format but it is marked with experimental and will not load with the current llvm-elfabi tool. Should generate something resembling:
--- !experimental-tapi-elf-v1 Arch: x86_64 Symbols: __Z16foo_default_visiii: { Type: Func } __Z6fvih_1ii: { Type: Func } __ZZ4fvihvE8fortytwo: { Type: Object, Size: 4 } __Z12someWeakFuncv: { Type: Func, Weak: true } ...
How does this look to you?
This really needs more tests. Please add a positive/negative test for the driver argument. Please try to organise the tests a bit to show what it is that they are testing, emission of public functions, not of protected functions or hidden functions. Behaviour with C++ classes. Behaviour with templates. I don't see any test for the vtable for classes that are public/private.
@compnerd Any other test cases you'd like before the first drop? @jakehehrlich @rupprecht Do things here look good to you guys? Both formats are marked experimental, I plan to work on a merge action next.
How about some cases for:
- global variable which is static and extern'ed
- global variable which is static defined in a function which is static
- global variable which is static defined in a function which is *not* inline
- global variable which is static which is static inline (GNU inline semantics)
- global variable which is static which is static inline (C99 inline semantics)
- virtual functions
- class vtables
- tests for hidden class deriving from hidden class
- tests for hidden class deriving from public class
- tests for public class deriving from public class
- tests for public class deriving from hidden class
- constexpr cases
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
---|---|---|
31 | would be nice to have a newline before the ctor |
Adding a test for inheritance and constructors/destroctors. Also addressing feedback from @alexshap
Can you upload this patch with context? Either use arc or upload w/ -U99999
I seem to have a lot of comments, but they're all nits -- my major concern being the llvm_unreachables should be errors instead (i.e. should be triggered in release builds).
Since this is clearly labeled as experimental, I think you should feel free to commit if you can get another lgtm (@compnerd?)
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3614–3616 | Args.getLastArgValue(options::OPT_iterface_stub_version_EQ) should already default to returning the empty string if unset (note the default param) | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
1678–1680 | (same here) | |
1688 | I think this is very much reachable if someone provides --interface-stub-version=x | |
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
21–22 | nit: these default initializations are redundant | |
27 | ditto | |
40–42 | Can you add a comment why these two are excluded? | |
42 | llvm::is_contained(Symbols, ND) | |
45–48 | This would be terser (and more readable) combined; if (isa<FieldDecl>(ND) || isa<ParamValDecl>(ND)) | |
96 | auto -> std::vector<std::string>, unless the return type is obvious to anyone that commits here (i.e. not me) | |
108 | auto -> std::string | |
207–208 | This should be an error, not llvm_unreachable (which I think gets filtered out in release builds) |
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
---|---|---|
243 | elsewhere it looks like this was changed to "ifs"? |
Thanks for the review.
I seem to have a lot of comments, but they're all nits -- my major concern being the llvm_unreachables should be errors instead (i.e. should be triggered in release builds).
Since this is clearly labeled as experimental, I think you should feel free to commit if you can get another lgtm (@compnerd?)
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3614–3616 | But what if it is set to something that's not either experimental-yaml-elf-v1 or experimental-tapi-elf-v1? This was to truncate any values that aren't those two to "" so that the error check could just be if (StubFormat.empty()). Maybe something other than a string switch would have been more explicit. | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
1688 | AH yes, you are right. This should probably be a diag. (clang/test/InterfaceStubs/bad-format.cpp tests for this "unreachable"). | |
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
42 | llvm::is_contained does not appear to work with std::map. I will try to figure out why. |
clang/lib/Frontend/CompilerInvocation.cpp | ||
---|---|---|
1690 | I think you could've used a llvm::Optional. | |
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
22 | An enumeration may be nicer. | |
40 | I tend to prefer if (RDO & ~FromTU) | |
57 | Why not if (ND->getVisibility() == DefaultVisibility) return true;? | |
66 | I think that using something like ignoreDecl is better than doBail. | |
69 | A newline before this would be nice. | |
73 | If it is a VarDecl, it cannot be a FunctionDecl. I think that this can be simplified a bit as: if (const auto *VD = dyn_cast<VarDecl>(VD)) return VD->getStorageClass() == StorageClass::SC_Extern || VD->getStorageClass() == StorageClass::SC_Static || VD->getParentFunctionOrMethod() == nullptr; | |
74 | Newline before this would be nice. I think that using auto here for the type is fine as you spelt out the type in the dyn_cast. | |
78 | I think that flipping this around would be nicer. if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) { ... } return FD->isInlined() && !Instance.getLangOpts().GNUInline; | |
80 | CTD is not used, please use isa and avoid the unused variable warning. | |
119 | Can we compress getMangledName and getMangledNames into getMangledNames and always return the vector? This avoids the duplication and simplifies the symbol recording later as well. | |
151 | You can probably hoist this to L129, and avoid the variable definition and the check on L131. |
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
---|---|---|
22 | For now the format goes into the first line of the yaml (in both formats) and ends up as "--- ! <Format>\n". | |
40 | 0 != (RDO & 0x1) vs (RDO & 0xE) I'm not sure if these are logically equivalent | |
73 | I don't understand. Here I remember wanting to bail if VD->getParentFunctionOrMethod() == nullptr and the vardecl was marked static. | |
78 | I think this still causes the isInlined + !GNUInlined code to trigger on CXXMethodDecls. |
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
---|---|---|
92 | I don't understand this clause. The structurors will always have 2 manglings (both itanium and MSVC use multiple, I don't remember the SUN and GNU variants off the top of my head). Adding the assert for the non-structor case is fine, but please wrap the entire thing in an LLVM_EXPENSIVE_ASSERT. | |
117 | This really should emit a diagnostic. The CompilerInstance should have a reference to the DiagnosticsEngine. |
cfe/trunk/lib/Frontend/CMakeLists.txt | ||
---|---|---|
58 ↗ | (On Diff #205203) | This is a layering issue. clangIndex depends on clangFrontend so clangFrontend should not depend on clangIndex. The circular dependency is allowed for static libraries but it breaks -DBUILD_SHARED_LIBS=on: CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle): "clangFrontend" of type SHARED_LIBRARY depends on "clangIndex" (weak) "clangIndex" of type SHARED_LIBRARY depends on "clangFrontend" (weak) At least one of these targets is not a STATIC_LIBRARY. Cyclic dependencies are allowed only among static libraries. |
cfe/trunk/lib/Frontend/CMakeLists.txt | ||
---|---|---|
58 ↗ | (On Diff #205203) | Is there a bot affected by this? I will take a look. |
cfe/trunk/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
---|---|---|
22 ↗ | (On Diff #205203) | Does StringSet<> work? |
36 ↗ | (On Diff #205203) | Are you relying on the ordered property of std::map? |
101 ↗ | (On Diff #205203) | .count |
103 ↗ | (On Diff #205203) | This should be a TODO |
124 ↗ | (On Diff #205203) | If you use a llvm container, emplace or try_emplace |
cfe/trunk/lib/Frontend/CMakeLists.txt | ||
---|---|---|
58 ↗ | (On Diff #205203) | Sadly there is no -DBUILD_SHARED_LIBS=True bot. This layering issue will break builds for all downstream users using that option though. |
I had to back out r363646. While it worked for me building locally with and without shared lib, it appears to have caused bots to break. I will take another look at fixing it in about 10 hours.
cfe/trunk/lib/Frontend/InterfaceStubFunctionsConsumer.cpp | ||
---|---|---|
12 ↗ | (On Diff #205203) | This file references symbols from clangIndex (#include "clang/Index/CodegenNameGenerator.h"), so you can't remove the clangIndex dependency from CMakeFiles.txt, otherwise -DBUILD_SHARED_LIBS=off builds would also fail. # Pass -Wl,-z,defs. This makes sure all symbols are defined. Otherwise a DSO # build might work on ELF but fail on MachO/COFF. The built shared library must have all of its dependencies specified on the linker command line. |
@plotfi Sorry I have to revert this patch. This can also cause problems in -DBUILD_SHARED_LIBS=off builds. clangFrontend files cannot #include "clang/Index/CodegenNameGenerator.h", I think you may have to move files around.
The tests for this code are failing in my build. Any suggestions welcome.
My setup:
cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=/usr/bin/clang-7 -DCMAKE_CXX_COMPILER=/usr/bin/clang++-7 -DCMAKE_C_FLAGS='-gmlt -Wall' -DCMAKE_CXX_FLAGS='-gmlt -Wall' -DLLVM_PARALLEL_LINK_JOBS=6 -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra' ../llvm-project/llvm
with head at:
commit 3b7668ae4bb8fabf95bd78dc6a06ca75f6ec3958 (HEAD, origin/master, origin/HEAD) Author: Matt Arsenault <Matthew.Arsenault@amd.com> Date: Mon Jul 1 13:34:26 2019 +0000 AMDGPU/GlobalISel: Improve icmp selection coverage. Select s64 eq/ne scalar icmp. llvm-svn: 364765
The failures:
ninja check-clang [3586/3587] Running the Clang regression tests llvm-lit: <...>/llvm-project/llvm/utils/lit/lit/llvm/config.py:340: note: using clang: <...>/build/bin/clang FAIL: Clang :: InterfaceStubs/virtual.cpp (6035 of 15113) ******************** TEST 'Clang :: InterfaceStubs/virtual.cpp' FAILED ******************** Script: -- : 'RUN: at line 2'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-TAPI <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp : 'RUN: at line 5'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-TAPI2 <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp : 'RUN: at line 8'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-SYMBOLS <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp -- Exit Code: 1 Command Output (stderr): -- <...>/llvm-project/clang/test/InterfaceStubs/virtual.cpp:16:23: error: CHECK-SYMBOLS-DAG: expected string not found in input // CHECK-SYMBOLS-DAG: NOTYPE GLOBAL HIDDEN {{.*}} _ZNK1Q5func1Ev ^ <stdin>:1:1: note: scanning from here There are 28 section headers, starting at offset 0x718: ^ <stdin>:4:25: note: possible intended match here [Nr] Name Type Address Off Size ES Flg Lk Inf Al ^ -- ******************** FAIL: Clang :: InterfaceStubs/class-template-specialization.cpp (6041 of 15113) ******************** TEST 'Clang :: InterfaceStubs/class-template-specialization.cpp' FAILED ******************** Script: -- : 'RUN: at line 2'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-TAPI <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp : 'RUN: at line 6'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-TAPI2 <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp : 'RUN: at line 9'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-SYMBOLS <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp -- Exit Code: 1 Command Output (stderr): -- <...>/llvm-project/clang/test/InterfaceStubs/class-template-specialization.cpp:26:23: error: CHECK-SYMBOLS-DAG: expected string not found in input // CHECK-SYMBOLS-DAG: FUNC GLOBAL DEFAULT {{[0-9]}} _Z1g ^ <stdin>:1:1: note: scanning from here There are 12 section headers, starting at offset 0x2a8: ^ <stdin>:7:41: note: possible intended match here [ 2] .text PROGBITS 0000000000000000 000040 000017 00 AX 0 0 16 ^ -- ******************** FAIL: Clang :: InterfaceStubs/hidden-class-inheritance.cpp (6044 of 15113) ******************** TEST 'Clang :: InterfaceStubs/hidden-class-inheritance.cpp' FAILED ******************** Script: -- : 'RUN: at line 2'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -DPARENT_CLASS_VISIBILITY="" -DCHILD_CLASS_VISIBILITY="" -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-X <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 7'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c -DPARENT_CLASS_VISIBILITY="" -DCHILD_CLASS_VISIBILITY="" -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-X-RE <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 13'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -DPARENT_CLASS_VISIBILITY=HIDDEN -DCHILD_CLASS_VISIBILITY="" -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-HP <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 18'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -DPARENT_CLASS_VISIBILITY=HIDDEN -DCHILD_CLASS_VISIBILITY="" -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-HP2 <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 23'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c -DPARENT_CLASS_VISIBILITY=HIDDEN -DCHILD_CLASS_VISIBILITY="" -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-HP-RE <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 29'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -DPARENT_CLASS_VISIBILITY="" -DCHILD_CLASS_VISIBILITY=HIDDEN -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-HC <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 34'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -DPARENT_CLASS_VISIBILITY="" -DCHILD_CLASS_VISIBILITY=HIDDEN -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-HC2 <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 39'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c -DPARENT_CLASS_VISIBILITY="" -DCHILD_CLASS_VISIBILITY=HIDDEN -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-HC-RE <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 45'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -DPARENT_CLASS_VISIBILITY=HIDDEN -DCHILD_CLASS_VISIBILITY=HIDDEN -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | <...>/build/bin/FileCheck -check-prefix=CHECK-HP-HC <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp : 'RUN: at line 50'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c -DPARENT_CLASS_VISIBILITY=HIDDEN -DCHILD_CLASS_VISIBILITY=HIDDEN -DPARENT_METHOD_VISIBILITY="" -DCHILD_METHOD_VISIBILITY="" <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-HP-HC-RE <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp -- Exit Code: 1 Command Output (stderr): -- <...>/llvm-project/clang/test/InterfaceStubs/hidden-class-inheritance.cpp:65:16: error: CHECK-X-RE: expected string not found in input // CHECK-X-RE: FUNC WEAK DEFAULT {{[0-9]+}} _ZN1C1mEv ^ <stdin>:1:1: note: scanning from here There are 48 section headers, starting at offset 0xdb0: ^ <stdin>:7:48: note: possible intended match here [ 2] .text PROGBITS 0000000000000000 000040 00005c 00 AX 0 0 16 ^ -- ******************** FAIL: Clang :: InterfaceStubs/visibility.cpp (6110 of 15113) ******************** TEST 'Clang :: InterfaceStubs/visibility.cpp' FAILED ******************** Script: -- : 'RUN: at line 2'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 -fvisibility=hidden <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | <...>/build/bin/FileCheck --check-prefix=CHECK-CMD-HIDDEN <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp : 'RUN: at line 6'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-yaml-elf-v1 -fvisibility=hidden <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | <...>/build/bin/FileCheck --check-prefix=CHECK-CMD-HIDDEN <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp : 'RUN: at line 10'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | <...>/build/bin/FileCheck --check-prefix=CHECK-CMD <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp : 'RUN: at line 14'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-yaml-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | <...>/build/bin/FileCheck --check-prefix=CHECK-CMD <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp : 'RUN: at line 18'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-tapi-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | <...>/build/bin/FileCheck --check-prefix=CHECK-CMD2 <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp : 'RUN: at line 22'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs -interface-stub-version=experimental-yaml-elf-v1 <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | <...>/build/bin/FileCheck --check-prefix=CHECK-CMD2 <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp : 'RUN: at line 26'; <...>/build/bin/clang -target x86_64-unknown-linux-gnu -o - -c <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp | llvm-readelf -s - 2>&1 | <...>/build/bin/FileCheck -check-prefix=CHECK-SYMBOLS <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp -- Exit Code: 1 Command Output (stderr): -- <...>/llvm-project/clang/test/InterfaceStubs/visibility.cpp:44:23: error: CHECK-SYMBOLS-DAG: expected string not found in input // CHECK-SYMBOLS-DAG: DEFAULT {{.*}} _Z10cmdVisiblev ^ <stdin>:1:1: note: scanning from here There are 9 section headers, starting at offset 0x2a8: ^ <stdin>:4:34: note: possible intended match here [Nr] Name Type Address Off Size ES Flg Lk Inf Al ^ -- ******************** Testing Time: 34.72s ******************** Failing Tests (4): Clang :: InterfaceStubs/class-template-specialization.cpp Clang :: InterfaceStubs/hidden-class-inheritance.cpp Clang :: InterfaceStubs/virtual.cpp Clang :: InterfaceStubs/visibility.cpp Expected Passes : 14989 Expected Failures : 21 Unsupported Tests : 99 Unexpected Failures: 4 FAILED: tools/clang/test/CMakeFiles/check-clang
where <...> stands for my local clang repo directory.
So I think I know what may be going on on your end. The llvm-readelf in your path I believe might be the wrong lllvm-readelf (llvm-readelf-7). Are you sure you built llvm-readelf from git?
Right -- no, I'm not. I explicitly added the llvm-7 bin to my path because without it, those tests fail with:
build/tools/clang/test/InterfaceStubs/Output/visibility.cpp.script: line 7: llvm-readelf: command not found
Sorry I didn't mention this up front. I've never needed llvm-readelf before -- what is the standard way to build/install from git?
Please don't do this. Your commit is wrong, and the right action from you is to revert it or fix it. I've fixed it for you here: rL364855
I'm also really not sure this is any good: why does clang look at ELF? In general I'd expect you to test something *way* earlier than ELF. Are you sure you're testing the right thing?
Ah, I must have mistakenly thought that other tools were already using llvm-readelf in cfe. Sorry about this.
This is in fact intended. The interface stubs feature needs to be verified against the actual symbols that are emitted into the elf binary. In the cases where I use llvm-readelf, I am using it to determine if the symbol is visible (to nm for example) but marked hidden in the .o file (but will end up being hidden in when linked).
Currently clang -emit-interface-stubs is not using any elf libraries, but it needs to verify against llvm-nm and/or llvm-readelf in these lit tests to determine if the visibility of the symbols generated in the text stubs is correct.
llvm-nm is already in the list of requirement for clang, so that would be fine to use. I'm not sure llvm-readelf is necessarily built for all targets, so my "fix" might still be broken. It's also another dependency, and a weird one at that. Can you test the property you're looking for in IR directly? And then test, in LLVM, that the same IR generates the ELF binary you want?
I am currently working on the next part of clang interface stubs that will take the interface stubs per compilation unit and merge them into one text stub (which will be used by something like llvm-elfabi to generate a stubbed out ELF .so file). I was using llvm-nm, but there are cases where the symbol is present in the .o file but it is marked as HIDDEN and llvm-readelf was what I was using to determine if the symbol was in fact marked as hidden. In these cases, the linker will not emit the symbol in the final .so file but it still needs the symbol as part of linking.
FWIW, It's possible that when I finish the merge job part of clang interface stubs I can remove this llvm-readelf test dependency because I can just check against llvm-nm that the merged text stub is correct.
I am currently working on the next part of clang interface stubs that will take the interface stubs per compilation unit and merge them into one text stub (which will be used by something like llvm-elfabi to generate a stubbed out ELF .so file). I was using llvm-nm, but there are cases where the symbol is present in the .o file but it is marked as HIDDEN and llvm-readelf was what I was using to determine if the symbol was in fact marked as hidden. In these cases, the linker will not emit the symbol in the final .so file but it still needs the symbol as part of linking.
This is similar to using "llvm-bcanalyzer" to check for serialization stuff from clang. We can't guarantee that to be available and adding a dep here would be weird. One way to workaround that would be to have a %llvm-readelf thing in lit and not run tests if not available (which also makes it questionable if it's not always running, but better than nothing?). Another option is to add a small clang tool that dumps what you need to check.
Looking at the code quickly, I'm not sure that this should be in clang itself. It sounds like a better fit for a clang-based tool, and not clang. Why does it need to be part of clang?
@jfb Having interface stubs generated by clang itself makes sense because we want to use the same clang invocation (with the addition of -emit-interface-stubs) to be what generates the stubs so that everything is fully consistent with what -fvisibility/__attribute__ visibility would have exposed in a normal non-stub build.
@jfb I think this could actually be refactored into a clang based tool come to think of it, but I am also looking at how to add some features to the driver so that it can support job pipelines that are different from the standard PP -> CC1 -> BE -> ASM -> LINK pipeline. I can work on a change once I sort out this pipeline work to move the interface generation into a clang tool so that instead of invoking clang -cc1 to generate the interfaces the clang driver invokes the new clang interface generation tool.
That sounds pretty good, thanks! Maybe someone like @ributzka / @arphaman / @Bigcheese have opinions on how to best do this.
Once you do this, can you also revert rL364855?
That is definitely possible, because then only the clang-tool that emits the interfaces needs to verify against llvm-readelf. The the driver invocation path can just test against the llvm-nm output of the final .so file. More collaboration and feedback on this project is very welcome.
Currently @compnerd and I have been discussing allowing for a more flexible compilation pipeline generation, where possibly instead of the standard PP -> CC -> BE -> ASM -> LNK we might have something like PP -> clang-ifsgen -> llvm-ifsmerge -> llvm-elfabi.
I thought that we were going to add experimental to this for the time being?