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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:500
@@ +499,3 @@
+    case OPT_script:
+      addFile(Arg->getValue(), true);
+      break;
----------------
Bigcheese wrote:
> 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.
Ah right. I'm fine with your change.

================
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");
----------------
Bigcheese wrote:
> 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.
My concern was mostly about the amount of code you added to this file. What we want to do here is to wrap a binary blob with an ELF header, and we are not interested in the details, so I wanted to move all these details to other file.

I see a room to simplify the writer thing, but as long as it is encapsulated into one file, we can improve the implementation independently from other file.

================
Comment at: ELF/SimpleELFWriter.h:31
@@ +30,3 @@
+public:
+  SimpleELFWriter(std::uint16_t Type, std::uint16_t Machine) {
+    memcpy(Header.e_ident, "\177ELF", 4);
----------------
Please create a .cpp file and move the definitions to that file.


https://reviews.llvm.org/D24060