Page MenuHomePhabricator

[lld][ELF] Add support for -b binary

Authored by Bigcheese on Aug 30 2016, 3:44 PM.



Implemented by building an ELF file in memory.

The -b formats 'elf', 'default', and 'binary' match gold behavior.

This is needed for a bunch of kernel drivers in FreeBSD.

Diff Detail


Event Timeline

Bigcheese updated this revision to Diff 69771.Aug 30 2016, 3:44 PM
Bigcheese retitled this revision from to [lld][ELF] Add support for -b binary.
Bigcheese updated this object.
Bigcheese added a reviewer: ruiu.
Bigcheese added a project: lld.
Bigcheese added a subscriber: llvm-commits.
ruiu added inline comments.Aug 30 2016, 4:52 PM
500 ↗(On Diff #69771)

I'd directly call readLinkerScript if OPT_script. It should allows you to remove the second parameter from addFile.

506 ↗(On Diff #69771)

We use S as a local variable name in other places for a return value of getValue.

515 ↗(On Diff #69771)

I think getSpelling always return -b, so I'd just print out "unknown -b format: ".

714 ↗(On Diff #69771)

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()));
1 ↗(On Diff #69771)

Writer is I think a confusing name because it sounds like an alternative Writer. It is actually rather a part of readers. I'd probably name this BinaryFile.h.

Bigcheese added inline comments.Aug 30 2016, 5:40 PM
500 ↗(On Diff #69771)

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())
MemoryBufferRef MBRef = *Buffer;

Which seems like more complexity than adding a param.

506 ↗(On Diff #69771)

Will do.

515 ↗(On Diff #69771)

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.

714 ↗(On Diff #69771)

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.

ruiu added inline comments.Aug 30 2016, 6:15 PM
500 ↗(On Diff #69771)

Ah right. I'm fine with your change.

714 ↗(On Diff #69771)

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.

31 ↗(On Diff #69771)

Please create a .cpp file and move the definitions to that file.

rafael added inline comments.
724 ↗(On Diff #69771)

These names look wrong. With gold and bfd I get


emaste added inline comments.Sep 7 2016, 11:16 AM
724 ↗(On Diff #69771)

I believe it should be _binary_ + alnum_pathname + {_start, _end, _size}, where alnum_pathname is the full pathname passed to the linker with non-alnum characters replaced by _.

I still thing that writing an ELF to read it back is a bad design, but the refactoring needed to avoid it is non-trivial. We are almost there for symbols, but sections require more work.

So please upload a last version with the linux build fixed, the symbol names changes and the nit in this comment. With that, it probably LGTM and will try to refactor it afterwards.

507 ↗(On Diff #69771)

You can write this as

case OPT_format: {
  StringRef Val = Arg->getValue();
  if (Val == "elf" || Val == "default")
    Config->Binary = false;
  else if (Val == "binary")
    Config->Binary = true;
    error("unknown " + Arg->getSpelling() + " format: " + Arg->getValue() +
          " (supported formats: elf, default, binary)");
Bigcheese updated this revision to Diff 70644.Sep 7 2016, 8:37 PM
  • Split SimpleELFWriter into .cpp
  • Fix nits
  • Fix linux build
  • Fix symbol names
rafael accepted this revision.Sep 8 2016, 4:35 AM
rafael added a reviewer: rafael.


1 ↗(On Diff #70644)

The test directory name is wrong. It should be ELF, not elf.

This revision is now accepted and ready to land.Sep 8 2016, 4:35 AM
emaste added a comment.Sep 8 2016, 8:54 AM

One issue remains in using this for the FreeBSD kernel module build -- we don't specify the output format / emulation right now, because we build GNU ld with the output target compiled-in. This results in an error (see D24348 for my suggested improvement to the error message).

I think it's much better if we don't have to build the linker multiple times though (once for each supported target). The right thing for us to do is make the build pass in -m.

ruiu added inline comments.Sep 8 2016, 9:19 AM
20 ↗(On Diff #70644)

I still don't like this file name. SimpleELFWriter sounds like it is for -o binary instead of -b. Can you rename BinaryFile.cpp?

736 ↗(On Diff #70644)

Please move these details to SimpleELFWriter.cpp.

emaste added a comment.Sep 8 2016, 1:26 PM

The right thing for us to do is make the build pass in -m.

That change is now in review for FreeBSD,

Bigcheese updated this revision to Diff 70753.Sep 8 2016, 2:30 PM
Bigcheese edited edge metadata.
  • Change name to ELFCreator
ruiu added inline comments.Sep 8 2016, 2:32 PM
736 ↗(On Diff #70753)

How about this?

Bigcheese added inline comments.Sep 8 2016, 2:35 PM
736 ↗(On Diff #70753)

Well, the code doesn't belong in ELFCreator.cpp. I can make a BinaryFile.cpp for it, but it's only 40 lines of code.

ruiu added inline comments.Sep 8 2016, 2:39 PM
736 ↗(On Diff #70753)

That distinction doesn't make much sense to me because this is part of the linker. If you have a concrete plan to move ELFCreator to some library, it may make sense, but it doesn't seem to happen soon.

silvas added a subscriber: silvas.Sep 8 2016, 3:14 PM
silvas added inline comments.
736 ↗(On Diff #70753)

Like Michael already said, ELFCreator is already used in at least 2 other places for PS4. It doesn't make sense to move this BinaryFile-specific code into ELFCreator.

ruiu added inline comments.Sep 8 2016, 5:17 PM
310 ↗(On Diff #70753)

Is this equivalent to std::unique_ptr<char *>? If so, please write that way.

Can you use uint8_t instead of char?

Bigcheese added inline comments.Sep 8 2016, 5:51 PM
310 ↗(On Diff #70753)

It needs to be [] so that delete[] is called instead of delete. But I'm actually going to change it to std::vector.

I can change it to uint8_t.

ruiu accepted this revision.Sep 9 2016, 2:57 PM
ruiu edited edge metadata.

LGTM. Let's land this. Thank you for doing this!

This revision was automatically updated to reflect the committed changes.