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

[Haowei Wu via Phabricator via cfe-commits]

haowei marked 3 inline comments as done.
haowei added a comment.

I will try to split this change.




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");

phosek wrote:
> Do we know whether the binary output format for the current target is ELF? 
> Shouldn't we check it first?
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?



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.

phosek wrote:
> 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?
I removed ELF constants and added helper functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99810

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 ) {
+static Error writeTBE(StringRef FilePath, IFSStub ) {
   // Write TBE to memory first.





Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:139
 
-  if (Stub->IfsVersion > IFSVersionCurrent)
-return make_error(
-"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> StubFromTBE =
+readIFSFromBuffer(FileReadBuffer->getBuffer());





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





Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:374
+  if (SOName.getNumOccurrences() == 1)
+Stub.SoName = SOName;
+  Optional 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits