[PATCH] D24060: [lld][ELF] Add support for -b binary

[Michael Spencer via llvm-commits]

Bigcheese added inline comments.

================
Comment at: ELF/Driver.cpp:500
@@ +499,3 @@
+    case OPT_script:
+      addFile(Arg->getValue(), true);
+      break;
----------------
ruiu wrote:
> I'd directly call `readLinkerScript` if OPT_script. It should allows you to remove the second parameter from `addFile`.
I was looking at that, but then I'd have to copy:


```
  if (Config->Verbose)
    outs() << Path << "\n";

  Optional<MemoryBufferRef> Buffer = readFile(Path);
  if (!Buffer.hasValue())
    return;
  MemoryBufferRef MBRef = *Buffer;
```

Which seems like more complexity than adding a param.

================
Comment at: ELF/Driver.cpp:506
@@ +505,3 @@
+    case OPT_format: {
+      StringRef Val = Arg->getValue();
+      if (Val == "elf" || Val == "default") {
----------------
ruiu wrote:
> We use `S` as a local variable name in other places for a return value of `getValue`.
Will do.

================
Comment at: ELF/Driver.cpp:515
@@ +514,3 @@
+      }
+      error("unknown " + Arg->getSpelling() + " format: " + Arg->getValue() +
+            " (supported formats: elf, default, binary)");
----------------
ruiu wrote:
> I think `getSpelling` always return `-b`, so I'd just print out `"unknown -b format: "`.
It can also return -format=.

Also I believe that would be bad practice in general. It makes it really easy to get into the situation where we print the "wrong" thing.

================
Comment at: ELF/InputFiles.cpp:714
@@ +713,3 @@
+std::unique_ptr<InputFile> BinaryFile::createELF() {
+  SimpleELFWriter<ELFT> ELF(ET_REL, Config->EMachine);
+  auto DataSec = ELF.addSection(".data");
----------------
ruiu wrote:
> Can you move all these details into SimpleELFWriter so that we can just do something like
> 
>   std::vector<uint8_t> Buf = createELFFromBinary(MB);
>   return createELFFile<ObjectFile>(MemoryBufferRef(Buf, MB.getBufferIdentifier()));
> 
If the concern is just having this much code in InputFile.cpp I'm fine with moving it somewhere else and calling it from here.

As for moving the code into SimpleELFWriter, I would rather keep SimpleELFWriter simple and not mix responsibilities. I plan to extend/refactor SimpleELFWriter to LLVMObject at some point and use it for final ELF emission for MC, split dwarf, objcopy (maybe), etc.

This was extracted (and reduced) from our internal version of the linker which has need of it elsewhere. So I'm already at 3 active uses of this code including this patch.


https://reviews.llvm.org/D24060