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

[Michael Spencer via llvm-commits]

On Wed, Aug 31, 2016 at 2:57 PM, Rui Ueyama <ruiu at google.com> wrote:

> I don't think it is that overkill because this is non-intrusive. Most
> parts of the linker can be agnostic on the presence of the -b feature with
> that approach.
>
> On Wed, Aug 31, 2016 at 12:28 PM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> 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
>>
>
I agree with Rui here. I looked at what it would take to do it without
creating an ELF file and it ends up touching a lot of code. Using the
existing ELF machinery makes this a very isolated change for a special case
feature. However if you see a simple way to implement it without ELF I'm
fine with doing so.

- Michael Spencer


>
>>
>> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160831/edffcebb/attachment.html>