This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by haowei on Apr 2 2021, 12:02 PM.

Details

Summary

This diff changes llvm-ifs to use unified IFS file format and perform other renaming changes in preparation for the merging between elfabi/ifs.

Diff Detail

Event Timeline

haowei created this revision.Apr 2 2021, 12:02 PM
haowei requested review of this revision.Apr 2 2021, 12:02 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 2 2021, 12:02 PM
haowei updated this revision to Diff 335038.Apr 2 2021, 3:44 PM

Addressed clang tests.

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 ↗(On Diff #335038)

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–156
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.

haowei updated this revision to Diff 335709.Apr 6 2021, 8:34 PM
haowei marked 5 inline comments as done.
haowei marked 3 inline comments as done.Apr 6 2021, 8:38 PM

I will try to split this change.

clang/lib/Driver/ToolChains/InterfaceStubs.cpp
26 ↗(On Diff #335038)

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.

haowei updated this revision to Diff 336213.Apr 8 2021, 1:50 PM
haowei marked an inline comment as done.
haowei retitled this revision from [ifs][elfabi] Merge llvm-elfabi tool into llvm-ifs to [ifs] Prepare llvm-ifs for elfabi/ifs merging..
haowei edited the summary of this revision. (Show Details)

Split this change into 2.

haowei updated this revision to Diff 343772.May 7 2021, 3:34 PM

Rebase the code to resolve presubmit errors.

phosek accepted this revision.Jun 14 2021, 12:55 AM
phosek added inline comments.
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.

This revision is now accepted and ready to land.Jun 14 2021, 12:55 AM
haowei updated this revision to Diff 352856.Jun 17 2021, 2:53 PM
haowei updated this revision to Diff 352857.
haowei marked 4 inline comments as done.
haowei marked an inline comment as done.Jun 17 2021, 2:58 PM
haowei added inline comments.
llvm/include/llvm/InterfaceStub/ELFObjHandler.h
24–25

This was renamed to ifs in D100139. Rename it here will need some changes in elfabi code, which is unnecessary (and will be deleted in D100139)

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.

phosek accepted this revision.Jun 18 2021, 6:08 PM

LGTM

llvm/lib/InterfaceStub/IFSStub.cpp
65

LG

70

This shouldn't be needed.

82

This shouldn't be needed.

haowei updated this revision to Diff 353498.Jun 21 2021, 2:48 PM
This revision was automatically updated to reflect the committed changes.
haowei marked 3 inline comments as done.
llvm/test/tools/llvm-ifs/write-stub.test