Page MenuHomePhabricator

[elfabi] Prepare llvm-elfabi for elfabi/ifs merging.
AcceptedPublic

Authored by haowei on Mar 25 2021, 9:58 PM.

Details

Summary

This change implements unified text stub format and command line interface proposed in the elfabi/ifs merge plan (https://lists.llvm.org/pipermail/llvm-dev/2021-March/149263.html). This CL mainly serves the purpose of verifying the ideas proposed in the merge plan. Once it lands, the next step will be update the clang IFSO driver and llvm-ifs to generate the text stub in the same format and performs the actual merging.

I ran into a lot of issues with LLVM's yaml library. It does not allow using ScalarTraits on Optional<uint16_t> type. It also does not allow mapping 2 traits on the same type, creating a lot of difficulties to support both Target: triple_string and Target: { Arch:..., Endianness: ...} in the same data structure. Please let me know if you have better workarounds than the one I used in this change.

When updating ifs tool with the unified text stub and command line interfacing, I plan to rename 'ELFStub.h' to 'InterfaceStub.h' 'TBEHandler.cpp' to 'IFSHandler.cpp' as these 2 will cover formats like TBD and COEF in the future. The naming of 'ELFObjHandler.h' will remans the same since it still only covers ELF related operations. Please let me know if you have any suggestions.

Diff Detail

Event Timeline

haowei created this revision.Mar 25 2021, 9:58 PM
haowei requested review of this revision.Mar 25 2021, 9:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 9:58 PM
haowei updated this revision to Diff 333500.Mar 25 2021, 10:15 PM

Address clang-tidy warnings.

I will take another look over this commit in the morning. Overall looks good.

llvm/lib/InterfaceStub/ELFStub.cpp
80

Can we populate this in this commit?

90

Can we populate this in this commit?

haowei updated this revision to Diff 334044.Mar 29 2021, 8:57 PM

Address test failures.

haowei marked 2 inline comments as done.Mar 29 2021, 8:58 PM
phosek added inline comments.Mar 30 2021, 12:18 AM
llvm/lib/InterfaceStub/ELFStub.cpp
80

StringSwitch should be more efficient than StringMap. It may be better to move this to llvm/lib/BinaryFormat/ELF.cpp and expose it as a helper function through llvm/include/llvm/BinaryFormat/ELF.h.

271

Ditto, I'd move this to llvm/lib/BinaryFormat/ELF.cpp.

llvm/lib/InterfaceStub/TBEHandler.cpp
138

Would this eventually become IFSVersion?

haowei updated this revision to Diff 334308.Mar 30 2021, 5:24 PM
haowei updated this revision to Diff 334310.Mar 30 2021, 5:29 PM
haowei marked 2 inline comments as done.
haowei added inline comments.
llvm/lib/InterfaceStub/ELFStub.cpp
80

StringSwitch should be more efficient than StringMap.

Is that because the map is relatively small, nor worth hashing it?

phosek added inline comments.Mar 30 2021, 5:47 PM
llvm/lib/BinaryFormat/ELF.cpp
207
llvm/lib/InterfaceStub/ELFStub.cpp
80

StringSwitch is written in a way that takes advantage of LLVM optimizations and should produce the most optimal code in this case.

plotfi added inline comments.Mar 30 2021, 6:02 PM
llvm/include/llvm/InterfaceStub/ELFStub.h
90

struct instead of class public?

llvm/lib/InterfaceStub/ELFObjHandler.cpp
564–569

Why both casts? How about one cast, or a static_cast<ELFArch> ?

llvm/lib/InterfaceStub/TBEHandler.cpp
50

default llvm_unreachable here?

54

Default StringSwitch case?

73

default llvm_unreachable?

79

Same as before. Default?

138

I think I would be OK with IFSVersion and TBE version sharing the same number.

198

Do we even support dynamic_casts in LLVM anymore? I might be missing something here, sorry.

haowei updated this revision to Diff 334549.Mar 31 2021, 3:09 PM
haowei marked 2 inline comments as done.
haowei marked 5 inline comments as done.Mar 31 2021, 3:18 PM
haowei added inline comments.
llvm/lib/InterfaceStub/TBEHandler.cpp
138

It will eventually be IFSVersion.

198

I tried dyn_cast seems not working. Not sure if dynamic_casts is allowed or not so I just change it static_cast.

plotfi added inline comments.Mar 31 2021, 4:13 PM
llvm/lib/InterfaceStub/TBEHandler.cpp
198

LLVM is compiled with no-rtti and no exceptions afaik.

alexshap added inline comments.
llvm/include/llvm/InterfaceStub/ELFStub.h
50
  1. explicit
  1. Name(std::move(SymbolName))
haowei updated this revision to Diff 334562.Mar 31 2021, 5:05 PM
haowei marked an inline comment as done.
plotfi added inline comments.Mar 31 2021, 5:07 PM
llvm/include/llvm/InterfaceStub/ELFStub.h
50

Does this need to be a std::move?

plotfi added inline comments.Mar 31 2021, 5:16 PM
llvm/include/llvm/InterfaceStub/ELFStub.h
50

Ah never mind, I see. Ignore my previous std::move comment.

phosek accepted this revision.May 3 2021, 12:24 PM

This looks good to me modulo a few nits although you may also want to wait for thumbs up from @plotfi and @compnerd.

llvm/lib/InterfaceStub/ELFObjHandler.cpp
253

This should be a C++ cast, probably a static_cast.

llvm/lib/InterfaceStub/TBEHandler.cpp
59

Does this trigger an error? If not we should maybe consider using an assertion.

97

Ditto here.

This revision is now accepted and ready to land.May 3 2021, 12:24 PM
plotfi accepted this revision.May 3 2021, 11:30 PM

LGTM once you've addressed @phosek's nits.

haowei updated this revision to Diff 343566.May 6 2021, 7:42 PM
haowei updated this revision to Diff 343567.
haowei marked 3 inline comments as done.
haowei added inline comments.
llvm/lib/InterfaceStub/TBEHandler.cpp
59

Yes, there is a test case in tools/llvm-elfabi/read-tbe-with-bad-endianness.test , Since the data is from a yaml file, it's better to generate an error message instead of using assertion.

97

there is a test case in tools/llvm-elfabi/read-tbe-with-bad-bitwidth.test

haowei updated this revision to Diff 343733.May 7 2021, 11:58 AM

Suppress clang-tidy warnings on YAML flow control variable.