This is an archive of the discontinued LLVM Phabricator instance.

[elfabi] Move elfabi related code to InterfaceStub library
ClosedPublic

Authored by haowei on Aug 10 2020, 11:55 AM.

Details

Summary

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust. This change moves ELFObjHandler and other elfabi code to a new InterfaceStub library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

Diff Detail

Event Timeline

haowei created this revision.Aug 10 2020, 11:55 AM
haowei requested review of this revision.Aug 10 2020, 11:55 AM
haowei edited the summary of this revision. (Show Details)Aug 10 2020, 11:59 AM
haowei added reviewers: phosek, mcgrathr, plotfi, compnerd.
phosek accepted this revision.Aug 10 2020, 2:51 PM

LGTM

This revision is now accepted and ready to land.Aug 10 2020, 2:51 PM
leonardchan accepted this revision.Aug 10 2020, 3:05 PM

LGTM for LLVM readability.

haowei updated this revision to Diff 284528.Aug 10 2020, 4:50 PM

correct formating.

haowei updated this revision to Diff 284851.Aug 11 2020, 12:32 PM
haowei retitled this revision from Move ELFObjHandler to TextAPI library to [elfabi] Move elfabi related code to IFS library.
haowei edited the summary of this revision. (Show Details)

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

We contacted plotfi and learned that llvm-ifs does not work well in production when it is used for elf stub generation. We suggested to improve llvm-ifs tools and unify it with llvm-elfabi tool as they share many similarities. llvm-elfabi has its own elf handler, most of its code have not yet landed and we will try our best to make it to production grade. This CL is the first step to strip elf handler away from llvm-elfabi and put it into a standalone library so we can continue work on it and make it shared by both tools without breaking anything.

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

In my first try I put it in TextAPI library and it causes a dependency loop as Object library is depends on TextAPI. Otherwise I think TextAPI is a better place for these files.

I am OK with InterfaceStub name. It explains the purpose clearer.

MaskRay added a comment.EditedAug 11 2020, 4:52 PM

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

In my first try I put it in TextAPI library and it causes a dependency loop as Object library is depends on TextAPI. Otherwise I think TextAPI is a better place for these files.

I think the sole reason that Object depends on TextAPI is due to D66159. Have you tried contacting the author about thinking whether we should continue organizing the two libraries this way?

I am OK with InterfaceStub name. It explains the purpose clearer.

If you look at existing library names. Abbreviation is not common, so IFS should probably be expanded.

I'd be fine with InterfaceStub as a name for the library.

llvm/lib/CMakeLists.txt
34 ↗(On Diff #284851)

This should be moved up to preserve the alphabetical sorting.

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

In my first try I put it in TextAPI library and it causes a dependency loop as Object library is depends on TextAPI. Otherwise I think TextAPI is a better place for these files.

I think the sole reason that Object depends on TextAPI is due to D66159. Have you tried contacting the author about thinking whether we should continue organizing the two libraries this way?

I am OK with InterfaceStub name. It explains the purpose clearer.

If you look at existing library names. Abbreviation is not common, so IFS should probably be expanded.

I think the reason to put TapiFile.cpp to Object library is legit. I would probably do the same thing if I were the original author.

haowei updated this revision to Diff 284948.Aug 11 2020, 6:43 PM
haowei retitled this revision from [elfabi] Move elfabi related code to IFS library to [elfabi] Move elfabi related code to InterfaceStub library.
haowei edited the summary of this revision. (Show Details)

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

We contacted plotfi and learned that llvm-ifs does not work well in production when it is used for elf stub generation. We suggested to improve llvm-ifs tools and unify it with llvm-elfabi tool as they share many similarities. llvm-elfabi has its own elf handler, most of its code have not yet landed and we will try our best to make it to production grade. This CL is the first step to strip elf handler away from llvm-elfabi and put it into a standalone library so we can continue work on it and make it shared by both tools without breaking anything.

I've spoken briefly with @phosek on this. I think I only mentioned that we used llvm-ifs for a production project internally, but that it has since been EOL'ed: this was unrelated to any of the technical decisions to use the yaml2obj lib at the time. For elf-stub generation I didn't have much trouble due to use of the yaml2obj lib though: generating the sections for ELF stubs is pretty straight forward, so yaml2obj worked fine for us.

The more tedious bits with Interface Stubs that I've run into are in things like clang driver and build system corner cases where llvm-ifs might expect some archive libFoo.a.ifs to be somewhere where it wasn't placed by the build system or the clang driver, also the clang stub file generation seems to sometimes not get the sizes of globals for a given symbol uniformly the same across the -cc1 invocations and its likely that the clang ASTMatcher side of this needs hardening. I ran into said issues when trying to build llvm with Clang Interface Stubs enabled.

Overall I am really happy to see an effort to unify elftapi, Darwin Tapi, and Interface Stubs though. My original plan was to use elftapi for the ELF generation, but it wasn't ready a year ago when I did this work but the yaml2obj stuff had been turned into a reusable library so thats really the reason I used yaml2obj back in Aug 2019.

I'd be fine with InterfaceStub as a name for the library.

@phosek @haowei Are there any plans in the near term to unify any of the TBD work with InterfaceStubs/elftapi?

@ributzka @cishida

I'd be fine with InterfaceStub as a name for the library.

@phosek @haowei Are there any plans in the near term to unify any of the TBD work with InterfaceStubs/elftapi?

@ributzka @cishida

We've talked about this some time ago and we can to the conclusion that it probably doesn't make sense to unify these for now because the overlap isn't as big as we originally thought it'd be. Specifically, .tbd defines a new object format that's understood by tools like the linker, but this is explicitly a non-goal for us. We see the textual representation only as an intermediate format that's used to generate the library stubs which from the perspective of other tools are just a regular ELF files.

phosek accepted this revision.Aug 12 2020, 12:33 PM

LGTM

llvm/lib/InterfaceStub/LLVMBuild.txt
1 ↗(On Diff #284948)

InterfaceStub

haowei updated this revision to Diff 285178.Aug 12 2020, 2:10 PM
haowei marked 2 inline comments as done.

Are there any more comments?

llvm/lib/InterfaceStub/LLVMBuild.txt
1 ↗(On Diff #284948)

Thanks for pointing out.

This revision was automatically updated to reflect the committed changes.
haowei marked an inline comment as done.
thakis added a subscriber: thakis.Aug 13 2020, 12:47 PM
thakis added inline comments.
llvm/lib/InterfaceStub/CMakeLists.txt
7 ↗(On Diff #285451)

Do you need this? Doesn't look like it?

haowei added inline comments.Aug 13 2020, 2:29 PM
llvm/lib/InterfaceStub/CMakeLists.txt
7 ↗(On Diff #285451)

Thanks for pointing out. Addressed in D85936

shayba added a subscriber: shayba.Aug 19 2020, 10:32 AM