This diff changes llvm-ifs to use unified IFS file format and perform other renaming changes in preparation for the merging between elfabi/ifs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This change is pretty large so I'm wondering if we could possibly split it into two changes, one that does the purely mechanical renaming of ELF and TBE to IFS, and second one that merges the two tools?
clang/lib/Driver/ToolChains/InterfaceStubs.cpp | ||
---|---|---|
26 | Do we know whether the binary output format for the current target is ELF? Shouldn't we check it first? | |
llvm/include/llvm/InterfaceStub/IFSStub.h | ||
28–52 | Since IFS is supposed to be binary format agnostic, it's a bit confusing to use ELF constants here. Could we instead define these values ourselves and convert them to the corresponding ELF values when generating the stub? | |
42 | ||
50 | ||
llvm/tools/llvm-elfabi/llvm-elfabi.cpp | ||
83 | ||
llvm/tools/llvm-ifs/llvm-ifs.cpp | ||
139 | ||
141 | ||
233 | ||
374 | The case difference on the left and right side (SoName vs SOName) is confusing, I'd unify them. | |
379–381 | No need for curly braces. |
I will try to split this change.
clang/lib/Driver/ToolChains/InterfaceStubs.cpp | ||
---|---|---|
26 | I tried following way: const bool WriteBin = !Args.getLastArg(options::OPT_emit_merged_ifs); if (WriteBin) { llvm::Triple::OSType OS = C.getDefaultToolChain().getEffectiveTriple().getOS(); switch(OS) { case llvm::Triple::OSType::DragonFly: case llvm::Triple::OSType::Fuchsia: case llvm::Triple::OSType::KFreeBSD: case llvm::Triple::OSType::Linux: case llvm::Triple::OSType::Lv2: case llvm::Triple::OSType::NetBSD: case llvm::Triple::OSType::OpenBSD: case llvm::Triple::OSType::Solaris: case llvm::Triple::OSType::Haiku: case llvm::Triple::OSType::Minix: case llvm::Triple::OSType::NaCl: case llvm::Triple::OSType::PS4: case llvm::Triple::OSType::ELFIAMCU: case llvm::Triple::OSType::Contiki: case llvm::Triple::OSType::Hurd: CmdArgs.push_back("--output-format=ELF"); break; // default: // // Not adding "--output-format" will cause llvm-ifs to crash. } } else { CmdArgs.push_back("--output-format=IFS"); } but it does not work. It looks like the Toolchain object is not properly constructed in this stage, which makes it difficult to determine the target of current clang invocation. Do you have any suggestions? | |
llvm/include/llvm/InterfaceStub/IFSStub.h | ||
28–52 | I removed ELF constants and added helper functions. |
llvm/include/llvm/InterfaceStub/ELFObjHandler.h | ||
---|---|---|
24–25 | Shouldn't this be in namespace ifs? | |
llvm/include/llvm/InterfaceStub/IFSStub.h | ||
82–83 | Use empty line between functions. | |
llvm/lib/InterfaceStub/IFSStub.cpp | ||
57–61 | This can be simplified. | |
65 | I'd consider using switch here so if someone adds a new entry to the IFSBitWidthType enum, it gets caught by the compiler. | |
69 | Ditto here. |
llvm/include/llvm/InterfaceStub/ELFObjHandler.h | ||
---|---|---|
24–25 | ||
llvm/lib/InterfaceStub/IFSStub.cpp | ||
65 | I changed it to switch and I added llvm_unreachable("unkown bitwidth"); to cases where an unknown bitwidth or endianness is used. Please let me know if it is OK to do so. The Unknown enum is reserved for ifs file that has unknown bitwidth or endianness field and will trigger ifs tool's early termination. So it shouldn't be seen when IFS library is writing an ELF file. |
Do we know whether the binary output format for the current target is ELF? Shouldn't we check it first?