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

[Rafael Espíndola via llvm-commits]

Creating an ELF file in memory seems way more than what is needed.
Even if we had a generic ELF writer in LLVM it would probably be an
overkill to use it for this.

Cheers,
Rafael


On 30 August 2016 at 21:15, Rui Ueyama via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits