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

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 31 14:57:00 PDT 2016


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
>
>
> 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/1cc8bdfb/attachment.html>


More information about the llvm-commits mailing list