[PATCH] D99810: [ifs][elfabi] Merge llvm-elfabi tool into llvm-ifs

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

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?



================
Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.cpp:26
   const bool WriteBin = !Args.getLastArg(options::OPT_emit_merged_ifs);
-  CmdArgs.push_back(WriteBin ? "write-bin" : "write-ifs");
+  CmdArgs.push_back(WriteBin ? "--output-format=ELF" : "--output-format=IFS");
   CmdArgs.push_back("-o");
----------------
Do we know whether the binary output format for the current target is ELF? Shouldn't we check it first?


================
Comment at: llvm/include/llvm/InterfaceStub/IFSStub.h:28-52
+enum class IFSSymbolType {
   NoType = ELF::STT_NOTYPE,
   Object = ELF::STT_OBJECT,
   Func = ELF::STT_FUNC,
   TLS = ELF::STT_TLS,
 
   // Type information is 4 bits, so 16 is safely out of range.
----------------
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?


================
Comment at: llvm/include/llvm/InterfaceStub/IFSStub.h:42
 
   // Endianness info is 1 bytes, 256 is safely out of rance.
   Unknown = 256,
----------------



================
Comment at: llvm/include/llvm/InterfaceStub/IFSStub.h:50
 
   // Bit width info is 1 bytes, 256 is safely out of rance.
   Unknown = 256,
----------------



================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:83
 /// of the YAML parser.
-static Error writeTBE(StringRef FilePath, ELFStub &Stub) {
+static Error writeTBE(StringRef FilePath, IFSStub &Stub) {
   // Write TBE to memory first.
----------------



================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:139
 
-  if (Stub->IfsVersion > IFSVersionCurrent)
-    return make_error<StringError>(
-        "IFS version " + Stub->IfsVersion.getAsString() + " is unsupported.",
-        std::make_error_code(std::errc::invalid_argument));
+  // Fall back to reading as a tbe.
+  if (InputFormat.getNumOccurrences() == 0 || InputFormat == FileFormat::IFS) {
----------------



================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:141
+  if (InputFormat.getNumOccurrences() == 0 || InputFormat == FileFormat::IFS) {
+    Expected<std::unique_ptr<IFSStub>> StubFromTBE =
+        readIFSFromBuffer(FileReadBuffer->getBuffer());
----------------



================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:233
+/// of the YAML parser.
+static Error writeTBE(StringRef FilePath, IFSStub &Stub) {
+  // Write TBE to memory first.
----------------



================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:374
+  if (SOName.getNumOccurrences() == 1)
+    Stub.SoName = SOName;
+  Optional<IFSArch> OverrideArch;
----------------
The case difference on the left and right side (`SoName` vs `SOName`) is confusing, I'd unify them.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:379-381
+  if (OptArch.getNumOccurrences() == 1) {
+    OverrideArch = ELF::convertArchNameToEMachine(OptArch.getValue());
+  }
----------------
No need for curly braces.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99810/new/

https://reviews.llvm.org/D99810