Page MenuHomePhabricator

[llvm-ifs] llvm Interface Stubs merging + object file generation tool.
ClosedPublic

Authored by plotfi on Sun, Aug 18, 11:49 PM.

Details

Summary

This tool merges interface stub files to produce either a merged interface stubs file or an ELF/COFF binary or tbe.
Currently it can write a merged interface stubs file or a merger of all the symbols into an ELF .so file.

The new IFS format is as follows:

--- !experimental-ifs-v1
IfsVersion:      1.0
Triple:          <llvm triple>
ObjectFileFormat: <ELF | TBD>
Symbols:
  _ZSymbolName: { Type: <type> }
...

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay added a subscriber: MaskRay.EditedMon, Aug 19, 12:03 AM

There is another tool tools/llvm-elfabi that does something very similar. (To be honest I don't quite get the point why that tool is required and why it has to be a llvm tool.) Nevertheless, it exists there, though not actively developed. Does this tool intend to replace/complement it?

There is another tool tools/llvm-elfabi that does something very similar. (To be honest I don't quite get the point why that tool is required and why it has to be a llvm tool.) Nevertheless, it exists there, though not actively developed. Does this tool intend to replace/complement it?

There was extensive discussion on https://reviews.llvm.org/D60974. The main purpose for llvm-ifs is because llvm-elfapi primarily deals with elf, and because we specifically do not want the mergeable interface stubs format to be the tbe format. Ideally, when the elfabi support for generating ELF binaries is finally landed, llvm-ifs will simply generate tbe in memory to product the elf. Ideally we want something that can generate ELF or COFF etc, and this is outside of the scope of llvm-elfabi.

plotfi edited the summary of this revision. (Show Details)Mon, Aug 19, 12:38 AM

This definitely needs tests.

llvm/tools/llvm-ifs/llvm-ifs.cpp
53 ↗(On Diff #215826)

Hmm, put this into an anonymous namespace and hoist it to the top of the file?

86 ↗(On Diff #215826)

Braces are unnecessary.

100 ↗(On Diff #215826)

Use a local variable for the Key.str(). You are creating two std::string here otherwise.

125 ↗(On Diff #215826)

SOName is better - its an initialism for Shared Object

260 ↗(On Diff #215826)

Probably should diagnose the actual mismatch so that its obvious why it doesn't match.

265 ↗(On Diff #215826)

What about conflicts?

plotfi updated this revision to Diff 216974.Fri, Aug 23, 4:11 PM
plotfi marked 4 inline comments as done.
  • Addressing @compnerd's feedback, still need to do some smarter conflict resolution for symbols and add tests.
  • Added rudimentary TBD support.
plotfi marked 2 inline comments as done.Fri, Aug 23, 4:11 PM
plotfi marked an inline comment as done.
plotfi added inline comments.
llvm/tools/llvm-ifs/llvm-ifs.cpp
207 ↗(On Diff #216974)

TODO: This is weird, I know. ELFSymbols for TBDs. I intend to fork away from ELFSymbol sometime soon.

plotfi marked an inline comment as done.Fri, Aug 23, 5:44 PM
plotfi added inline comments.
llvm/tools/llvm-ifs/LLVMBuild.txt
21 ↗(On Diff #216974)

TODO: Add ObjectYAML

plotfi edited the summary of this revision. (Show Details)Fri, Aug 23, 5:44 PM
plotfi updated this revision to Diff 217244.Mon, Aug 26, 2:32 PM

Adding some tests. and added some really basic symbol conflict error checking (no resolution though).

plotfi updated this revision to Diff 217269.Mon, Aug 26, 4:53 PM

adding another test, to merge objects and funcs of different sizes.

Probably is worth having a test for the error case as well.

llvm/test/tools/llvm-ifs/a.ifs
22 ↗(On Diff #217269)

Why are they undefined?

llvm/test/tools/llvm-ifs/b.ifs
20 ↗(On Diff #217269)

Why undefined?

llvm/test/tools/llvm-ifs/bd.ifs
1 ↗(On Diff #217269)

Not sure what the value is of this test. You already tested 1 file, and 1+ files.

llvm/test/tools/llvm-ifs/c.ifs
1 ↗(On Diff #217269)

Same.

llvm/test/tools/llvm-ifs/class-template-specialization.ifs
1 ↗(On Diff #217269)

Okay, I suppose that this is C++. At the very least, emit a TBD to ensure that the user label prefix is handled correctly.

llvm/test/tools/llvm-ifs/ef.ifs
28 ↗(On Diff #217269)

Why undefined

llvm/test/tools/llvm-ifs/object.ifs
1 ↗(On Diff #217269)

Objects were already tested previously.

llvm/test/tools/llvm-ifs/template-namespace-function.ifs
1 ↗(On Diff #217269)

Not sure what this is testing.

llvm/test/tools/llvm-ifs/virtual.ifs
1 ↗(On Diff #217269)

Not sure what the virtuality testing is actually testing.

llvm/test/tools/llvm-ifs/visibility.ifs
1 ↗(On Diff #217269)

Not sure if this is valuable.

I suppose that the test cases that I think are valuable:

  • 1 file
  • 1+ file
  • functions
  • objects
  • functions w/ different alignments
  • objects w/ different alignments
  • functions w/ different sizes
  • objects w/ different sizes
  • functions w/ different alignments, sizes
  • objects w/ different alignments, sizes
  • weak symbols
  • mismatch (error cases)
plotfi updated this revision to Diff 217327.Tue, Aug 27, 2:17 AM

test updates based on feedback

plotfi marked 6 inline comments as done.Tue, Aug 27, 2:18 AM
plotfi marked 2 inline comments as done.Tue, Aug 27, 2:24 AM
plotfi added inline comments.
llvm/test/tools/llvm-ifs/ef.ifs
28 ↗(On Diff #217269)

Undefined because I do not define a .text section for the symbol to belong to. I could do something like this to name the undefined go away:

--- !ELF
FileHeader:
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_REL
  Machine:         EM_X86_64
Sections:
  - Name:            .text
    Type:            SHT_PROGBITS
Symbols:
  Global:
    - Name:            _Z3foov
      Type:            STT_FUNC
      Section:         .text
...
plotfi updated this revision to Diff 217438.Tue, Aug 27, 11:07 AM
plotfi marked an inline comment as done.
plotfi marked 3 inline comments as done.Tue, Aug 27, 11:12 AM
plotfi added inline comments.
llvm/test/tools/llvm-ifs/class-template-specialization.ifs
1 ↗(On Diff #217269)

I think in the first version, TBD is experimental. Ideally any IFS generation for Darwin TBDs would have come from a .*-apple-.* triple of some sort where clang generated the correct name manglings. Otherwise, there'd need to be some kind of demangler-remangler at play (which also wouldn't work as we've discussed for win32 mangling).

plotfi updated this revision to Diff 217460.Tue, Aug 27, 11:57 AM

Added some better conflict checking tests.

plotfi marked an inline comment as done.Tue, Aug 27, 1:07 PM
plotfi updated this revision to Diff 217484.Tue, Aug 27, 1:20 PM

Checking for conflicts in the Ifs header.

plotfi updated this revision to Diff 217497.Tue, Aug 27, 2:30 PM

More version checks

I think it may make some sense to strip the user label prefix from the symbols that are listed in the interface, and then add that in when generating the ELF/TBD.

llvm/test/tools/llvm-ifs/a-conflict-header-version.ifs
7 ↗(On Diff #217497)

Does it matter if the version is mismatched between the two versions as long as you can deserialise and merge?

llvm/test/tools/llvm-ifs/a-conflict-type.ifs
13 ↗(On Diff #217497)

Missing space before the }.

llvm/test/tools/llvm-ifs/a-conflict-undefined.ifs
6 ↗(On Diff #217497)

Why do we even need undefined symbols for the interfaces? Interfaces should be fully defined no?

llvm/test/tools/llvm-ifs/a-conflict-weak.ifs
7 ↗(On Diff #217497)

mmm...I think that we should verify that this is appropriate behaviour. I think that the linker will normally accept the weak definition and replace it with the strong definition and we should just upgrade to the strong definition.

llvm/test/tools/llvm-ifs/b-conflict-size.ifs
13 ↗(On Diff #217497)

Missing space before }.

plotfi updated this revision to Diff 217685.Wed, Aug 28, 10:44 AM
plotfi marked an inline comment as done.
  • Dropping vestigial ELFSymbols, now I have IFSSymbol.
  • addressed @compnerd 's feedback
plotfi marked 6 inline comments as done.Wed, Aug 28, 10:46 AM
plotfi added inline comments.
llvm/test/tools/llvm-ifs/a-conflict-header-version.ifs
7 ↗(On Diff #217497)

I think what i'll do is, if the major versions are different then error out but if the minors are different then resolve any difference.

llvm/test/tools/llvm-ifs/a-conflict-type.ifs
13 ↗(On Diff #217497)

Aye

llvm/test/tools/llvm-ifs/a-conflict-weak.ifs
7 ↗(On Diff #217497)

Can I add this resolution in a followup? it was my intention to do the more complicated resolution in a followup just so the patches stay simpler

plotfi updated this revision to Diff 217928.Thu, Aug 29, 10:12 AM
plotfi marked 3 inline comments as done.

adding .data and .rodata to elf sections

Please rename the tests to be more meaningful so that others can possibly identify what they are meant to test and can address things which may break when other changes are made.

llvm/test/tools/llvm-ifs/a-conflict-type.ifs
6 ↗(On Diff #217928)

Should we diagnose the types which are conflicting?

llvm/test/tools/llvm-ifs/a-conflict-weak.ifs
7 ↗(On Diff #217928)

The diagnostic is difficult to understand. Perhaps make this similar to the diagnostic for the mismatched types, and indicate that the failure is that the symbol is weak.

llvm/tools/llvm-ifs/llvm-ifs.cpp
40 ↗(On Diff #217928)

Why no long name?

44 ↗(On Diff #217928)

Same

52 ↗(On Diff #217928)

Same

plotfi updated this revision to Diff 217971.Thu, Aug 29, 2:04 PM
plotfi marked 3 inline comments as done.

better diag, better test names

llvm/test/tools/llvm-ifs/a-conflict-type.ifs
6 ↗(On Diff #217928)

yes.

compnerd accepted this revision.Fri, Aug 30, 8:53 AM
compnerd added inline comments.
llvm/test/tools/llvm-ifs/class-template-specialization.ifs
1 ↗(On Diff #217971)

Is there anything special about C++ template specialization for the merge? I couldn't spot any. Perhaps this should be called c++/cxx or some such?

This revision is now accepted and ready to land.Fri, Aug 30, 8:53 AM
plotfi marked 2 inline comments as done.Fri, Aug 30, 9:53 AM
plotfi added inline comments.
llvm/test/tools/llvm-ifs/class-template-specialization.ifs
1 ↗(On Diff #217971)

Hmm, actually I think the weak test might be sufficient to test something that actually came out of clang. I could drop this one.

plotfi updated this revision to Diff 218124.Fri, Aug 30, 10:11 AM
plotfi marked an inline comment as done.

longer flags, no more =

plotfi marked 2 inline comments as done.Fri, Aug 30, 10:11 AM
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
llvm/trunk/test/lit.cfg.py
25

Since this list doesn't contain ".ifs", none of the tests you added actually run as part of check-llvm. I'd recommend renaming the tests to ".test" instead of adding ".ifs" to this list.

thakis added inline comments.Tue, Sep 3, 6:18 AM
llvm/trunk/test/lit.cfg.py
25

A possibly even better fix is to add test/tools/llvm-ifs/lit.local.cfg that does config.suffixes = ['.ifs']. See e.g. llvm/test//TableGen/lit.local.cfg for an example.

plotfi marked an inline comment as done.Tue, Sep 3, 10:44 AM
plotfi added inline comments.
llvm/trunk/test/lit.cfg.py
25

Thank you for this heads up Nico. I'll land something shortly. I like the config.suffixes approach.

cishida added inline comments.Wed, Sep 4, 3:25 PM
llvm/trunk/tools/llvm-ifs/llvm-ifs.cpp
238

I apologize for commenting on this retroactively, but if this hardcoded value is related to the experimental support for TBD files, I think there should be a comment detailing it.

plotfi marked 2 inline comments as done.Wed, Sep 4, 10:11 PM
plotfi added inline comments.
llvm/trunk/tools/llvm-ifs/llvm-ifs.cpp
238

Ah yeah, thanks for bringing this up @cishida. I’ll try and at the very least add some more comments on this soon. For now I’m still working on wiring up the clang driver and ifs generation to llvm-ifs.

plotfi marked 2 inline comments as done.Wed, Sep 4, 10:11 PM