Page MenuHomePhabricator

Clang IFSO driver action.
Needs ReviewPublic

Authored by plotfi on Apr 22 2019, 11:24 AM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
plotfi added inline comments.May 9 2019, 1:44 PM
clang/lib/Frontend/FrontendActions.cpp
223 ↗(On Diff #198157)

HandleNamedDecl bails when the NamedDecl is not a NamedDecl.

plotfi marked 2 inline comments as done.May 9 2019, 1:53 PM
plotfi updated this revision to Diff 198902.May 9 2019, 2:02 PM

addressing a lot of @compnerd 's feedback. Still lots of templating.

plotfi marked 8 inline comments as done.May 9 2019, 2:04 PM
plotfi updated this revision to Diff 198963.May 9 2019, 5:49 PM
plotfi marked 9 inline comments as done.

removed all the template functions to handle decls.

plotfi marked 7 inline comments as done.May 9 2019, 5:52 PM
plotfi updated this revision to Diff 198967.May 9 2019, 6:20 PM

cleaned up VisitNamedDecl

plotfi marked an inline comment as done.May 9 2019, 6:22 PM
plotfi updated this revision to Diff 198977.May 9 2019, 8:17 PM

renaming things from "ifso" to "interface stubs"

plotfi marked an inline comment as done.May 9 2019, 9:38 PM
plotfi added inline comments.
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?

plotfi marked 2 inline comments as done.May 10 2019, 11:58 AM
plotfi added inline comments.
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.

plotfi marked an inline comment as done.May 10 2019, 12:10 PM

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.

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.

plotfi updated this revision to Diff 199058.May 10 2019, 12:43 PM

Changing !tapi-tbe to !experimental-tapi-elf-v1, removing Endianness field from tapi-tbe.

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.

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.

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.

Agreed. Adding.

plotfi updated this revision to Diff 199112.May 10 2019, 5:20 PM

Adding lit test cases.

@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
plotfi updated this revision to Diff 200393.Mon, May 20, 8:33 PM

adding a nice inline test

plotfi updated this revision to Diff 200602.Tue, May 21, 3:50 PM

fixing support for static functions.

plotfi updated this revision to Diff 200805.Wed, May 22, 11:56 AM

adding static/extern tests.

plotfi updated this revision to Diff 200828.Wed, May 22, 3:24 PM
plotfi updated this revision to Diff 201104.Thu, May 23, 5:22 PM

Improving visibility extraction for class template specializations.

plotfi updated this revision to Diff 201339.Fri, May 24, 2:38 PM

More test improvement.

plotfi updated this revision to Diff 201471.Sun, May 26, 3:41 PM

virtual function test

alexshap added inline comments.Mon, Jun 3, 2:26 PM
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp
31

would be nice to have a newline before the ctor

plotfi updated this revision to Diff 202976.Tue, Jun 4, 10:18 AM

Adding a test for inheritance and constructors/destroctors. Also addressing feedback from @alexshap

plotfi marked an inline comment as done.Tue, Jun 4, 10:18 AM
plotfi updated this revision to Diff 203464.Thu, Jun 6, 4:23 PM

Updated hidden parent/child class inheritance cases.

plotfi updated this revision to Diff 203465.Thu, Jun 6, 4:26 PM

git mv'ed hidden-class-inheritance.cpp

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)

243

elsewhere it looks like this was changed to "ifs"?

plotfi updated this revision to Diff 203638.Fri, Jun 7, 3:43 PM

-U999999

plotfi updated this revision to Diff 204187.Tue, Jun 11, 4:04 PM
plotfi marked 11 inline comments as done.

addressing @rupprecht's feedback

plotfi marked 3 inline comments as done.Tue, Jun 11, 4:05 PM

Can you upload this patch with context? Either use arc or upload w/ -U99999

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.

plotfi updated this revision to Diff 204308.Wed, Jun 12, 9:26 AM
plotfi marked 2 inline comments as done.

Improving support for visibility correctness with classes, methods and inheritance.

compnerd added inline comments.Wed, Jun 12, 9:55 AM
clang/lib/Frontend/CompilerInvocation.cpp
1691

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.

plotfi updated this revision to Diff 204978.Sun, Jun 16, 5:06 PM
plotfi marked 17 inline comments as done.

Addressing @compnerd's feedback

plotfi added inline comments.Sun, Jun 16, 5:07 PM
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".
I'd like to keep it this way for now.

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.