This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by plotfi on Aug 18 2019, 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

Event Timeline

plotfi created this revision.Aug 18 2019, 11:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added a subscriber: MaskRay.EditedAug 19 2019, 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)Aug 19 2019, 12:38 AM

This definitely needs tests.

llvm/tools/llvm-ifs/llvm-ifs.cpp
54

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

87

Braces are unnecessary.

101

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

126

SOName is better - its an initialism for Shared Object

261

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

266

What about conflicts?

plotfi updated this revision to Diff 216974.Aug 23 2019, 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.Aug 23 2019, 4:11 PM
plotfi marked an inline comment as done.
plotfi added inline comments.
llvm/tools/llvm-ifs/llvm-ifs.cpp
207

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.Aug 23 2019, 5:44 PM
plotfi added inline comments.
llvm/tools/llvm-ifs/LLVMBuild.txt
21

TODO: Add ObjectYAML

plotfi edited the summary of this revision. (Show Details)Aug 23 2019, 5:44 PM
plotfi updated this revision to Diff 217244.Aug 26 2019, 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.Aug 26 2019, 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.Aug 27 2019, 2:17 AM

test updates based on feedback

plotfi marked 6 inline comments as done.Aug 27 2019, 2:18 AM
plotfi marked 2 inline comments as done.Aug 27 2019, 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.Aug 27 2019, 11:07 AM
plotfi marked an inline comment as done.
plotfi marked 3 inline comments as done.Aug 27 2019, 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.Aug 27 2019, 11:57 AM

Added some better conflict checking tests.

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

Checking for conflicts in the Ifs header.

plotfi updated this revision to Diff 217497.Aug 27 2019, 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.Aug 28 2019, 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.Aug 28 2019, 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.Aug 29 2019, 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
41

Why no long name?

45

Same

53

Same

plotfi updated this revision to Diff 217971.Aug 29 2019, 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.Aug 30 2019, 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.Aug 30 2019, 8:53 AM
plotfi marked 2 inline comments as done.Aug 30 2019, 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.Aug 30 2019, 10:11 AM
plotfi marked an inline comment as done.

longer flags, no more =

plotfi marked 2 inline comments as done.Aug 30 2019, 10:11 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 3 2019, 6:09 AM
thakis added inline comments.
llvm/trunk/test/lit.cfg.py
25 ↗(On Diff #218138)

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.Sep 3 2019, 6:18 AM
llvm/trunk/test/lit.cfg.py
25 ↗(On Diff #218138)

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.Sep 3 2019, 10:44 AM
plotfi added inline comments.
llvm/trunk/test/lit.cfg.py
25 ↗(On Diff #218138)

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

cishida added inline comments.Sep 4 2019, 3:25 PM
llvm/trunk/tools/llvm-ifs/llvm-ifs.cpp
238 ↗(On Diff #218138)

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.Sep 4 2019, 10:11 PM
plotfi added inline comments.
llvm/trunk/tools/llvm-ifs/llvm-ifs.cpp
238 ↗(On Diff #218138)

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.Sep 4 2019, 10:11 PM
nickdesaulniers added inline comments.
llvm/trunk/test/tools/llvm-ifs/conflict-size.ifs
15 ↗(On Diff #218138)

Aplogies for asking on this patch (and very late after it was submitted), but playing with llvm-ifs, I see the following error running llvm-ifs against my copy of glibc.

$ llvm-ifs /lib/x86_64-linux-gnu/libc.so.6  --output-format=ELF -o -      
error: Interface Stub: Size Mismatch for sys_sigabbrev.
Filename: /lib/x86_64-linux-gnu/libc.so.6
Size Values: 512 520

$ llvm-readelf -s /lib/x86_64-linux-gnu/libc.so.6| grep sys_sigabbrev
   881: 00000000001e9d00   512 OBJECT  GLOBAL DEFAULT    27 sys_sigabbrev@GLIBC_2.2.5
   882: 00000000001e9d00   520 OBJECT  GLOBAL DEFAULT    27 sys_sigabbrev@@GLIBC_2.3.3

So it looks like glibc has 2 different sizes of sys_sigabbrev, but the symbols seem versioned, IIUC? ie. one is sys_sigabbrev@GLIBC_2.2.5 vs sys_sigabbrev@@GLIBC_2.3.3.

Is there an option to pick one, or perhaps both without removing the symbol versioning?