Page MenuHomePhabricator

[ELF2] Handle -m option
ClosedPublic

Authored by denis-protivensky on Sep 22 2015, 5:45 AM.

Details

Summary

Parse and apply emulation given with -m option.
Check input files to match ELF type and machine architecture provided with -m.

Add positive tests for supported emulations.
Add negative test with unknown emulation.
Rework tests with incompatible emulation and input files.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
emaste added a subscriber: davide.Sep 23 2015, 6:08 AM
emaste added inline comments.
ELF/Config.h
39 ↗(On Diff #35475)

Do we still need an EM_X86_64 default?

test/elf2/emulation.s
1 ↗(On Diff #35475)

It would be nice to have some non-Linux triples as well (e.g. x86_64-unknown-freebsd). @davide or I could add some in a subsequent commit.

rafael added inline comments.Sep 23 2015, 6:29 AM
ELF/Config.h
26 ↗(On Diff #35475)

git-clang-format the patch.

Function names start with a lowercase letter.

ELF/Driver.cpp
88 ↗(On Diff #35475)

Variable names start with an upper case letter.

96 ↗(On Diff #35475)

This fails to build:

Driver.cpp:96:12: error: no viable conversion from returned value of type 'unique_ptr<lld::elf2::ELFFileBase>' to function return type 'unique_ptr<lld::elf2::InputFile>'

test/elf2/incompatible.s
8 ↗(On Diff #35475)

That is a regression. When two things are incompatible we should still say what they are. An object file can be incompatible with another or with a -m option.

ELF/Config.h
39 ↗(On Diff #35475)

It'll be replaced with a reasonable value during linking. Do you have ideas what to put here? EM_NONE?

test/elf2/emulation.s
1 ↗(On Diff #35475)

Thanks.

Updated:

  • Fix style
  • Fix clang build error
  • Provide source of target incompatibility
rafael added inline comments.Sep 23 2015, 8:51 AM
ELF/Config.h
14 ↗(On Diff #35501)

You only need Support/ELF.h

15 ↗(On Diff #35501)

Not used

35 ↗(On Diff #35501)

Can these two variables be left uninitialized? If not, please make them an explicit Optional<>. We really should not give EM_X86_64 or ELF64LE any special treatment.

ELF/SymbolTable.cpp
25 ↗(On Diff #35501)

git-clang-format

rafael added inline comments.Sep 23 2015, 9:03 AM
ELF/Driver.cpp
18 ↗(On Diff #35501)

Unnecessary.

40 ↗(On Diff #35501)

Replace this with a

using namespace llvm::ELF;

just after the existing

using namespace llvm;

80 ↗(On Diff #35501)

Drop the inline.

You can avoid creating the low level getElfMachineType by opening the
elf files earlier (see the attached patch).

ruiu added a subscriber: ruiu.Sep 23 2015, 3:50 PM
ruiu added inline comments.
ELF/Driver.cpp
44 ↗(On Diff #35501)

ElfKind -> ELFKind

67 ↗(On Diff #35501)

Passing an in-memory object as a StringRef feels a bit strange. Maybe you want to passs MemoryBuffer instead.

76–77 ↗(On Diff #35501)

No need for else because the other path ends with return.

81–82 ↗(On Diff #35501)

This function is probably too short to be defined as a function. Just write two lines of code where you call this function.

96 ↗(On Diff #35501)

I'd remove this TargetDefined variable. Instead, I'd change the type of Config->ELFKind to Optional<>, and check for the existence of the value instead of a separate flag.

103 ↗(On Diff #35501)

Looks like TargetSource is used just for printing this error message. Can you remove that? I think a detailed error message is generally good, but this error message is used rarely, and probably that wouldn't match the cost of having another variable here. Then we can keep createELFFile to take only one argument just like before.

114 ↗(On Diff #35501)

Remove this

116 ↗(On Diff #35501)

Let's keep the original function name.

117 ↗(On Diff #35501)

and this blank lines.

121 ↗(On Diff #35501)

Move the definition of Emulations inside here as a static local constant because only this function uses that.

@rafael, thanks for the patch, but I'd prefer to leave the getElfMachineType method since it allows to simplify the interface of ELFData class and ObjectFile/SharedFile. It removes bunch of methods from the .cpp file as well.

ELF/Config.h
14 ↗(On Diff #35501)

Agreed.

15 ↗(On Diff #35501)

I introduced the variable of uint16_t type, so this header is used and the style guide doesn't prohibit adding headers to avoid hidden dependencies.

35 ↗(On Diff #35501)

Will make ElfKind optional and set EMachine to EM_NONE.

ELF/Driver.cpp
18 ↗(On Diff #35501)

I'll change this to ELFTypes.h because it contains Elf_Ehdr_Impl definition.

44 ↗(On Diff #35501)

This causes build errors with gcc, that's why I made it like that.

67 ↗(On Diff #35501)

Will try.

76–77 ↗(On Diff #35501)

Ok.

80 ↗(On Diff #35501)

Is this something style-guide related or error-prone? Why to drop it?

81–82 ↗(On Diff #35501)

I'm against the code duplication in general. Even for two-liner, so I'd like to leave it here.

96 ↗(On Diff #35501)

Good.

103 ↗(On Diff #35501)

That was my original design, but @rafael insisted we need this to avoid test regression. See comments for incompatible.s test below.

116 ↗(On Diff #35501)

Ok.

121 ↗(On Diff #35501)

Good point.

Maybe, but that is independent and pretty big, so it should be in another patch.

Agreed.

ruiu added inline comments.Sep 24 2015, 11:17 AM
ELF/Config.h
35 ↗(On Diff #35501)

Or you could define a new enum, ELFNoneKind and use the value as an indicator of absence of a valid value.

ELF/Driver.cpp
80 ↗(On Diff #35501)

The rule of thumb is we shouldn't write inline unless it's proved to be necessary. Basically this piece of code doesn't have to be optimized, or even the entire Driver doesn't have to be fast, because the runtime cost of Driver is negligible after all. I wouldn't even think about speed when writing the Driver to save my brain power to focus on something more important. :)

81–82 ↗(On Diff #35501)

Well, that depends. If they are just one-line assignment statement to a Config member variable, you wouldn't make the "duplicate" assignments to a function. Two assignments are still more readable than a function call.

103 ↗(On Diff #35501)

I feel like there must be some way to remove this parameter. Can we do this check not in this function but at Driver.cpp line 189?

ELF/Config.h
35 ↗(On Diff #35501)

Better to have the optional, since defining one enum value used in only one case as a default looks like bad design.

ELF/Driver.cpp
67 ↗(On Diff #35501)

MemoryBufferRef doesn't have suitable methods to operate on bytes, so I'll in any case need to construct StringRef from MemoryBufferRef inside. No profit in changing the signature, especially for non-public method.

103 ↗(On Diff #35501)

Thought about that, but it would be worse: we'll have two logically equal checks of having ELFKind set in two different places. Moreover, we'll need to return nullptr value from the method and then check it in the loop. I don't feel it's worth doing.

ruiu added inline comments.Sep 25 2015, 3:12 PM
ELF/Config.h
35 ↗(On Diff #35501)

No, it's not always bad, but sometimes good. Absence of a valid value is often represented using a null pointer, for example.

ELF/Driver.cpp
67 ↗(On Diff #35501)

Why I felt that was a bit weird is because when I see a StringRef, I expect it would refer to some text string and not to arbitrary byte sequence (if that's the case, ArrayRef<uint8_t> would be suitable). So I'd think we should change the signature. You can call getBuffer() *inside* this function and not before calling this function.

denis-protivensky marked 20 inline comments as done.

Updated:

  • Make ElfKind optional
  • Move Emulations to the place of use
  • Rename createTargetELFFile -> createELFFile
  • Code style fixes
  • Minimize the number of changes by excluding parts that may be removed
ruiu added inline comments.Sep 28 2015, 7:03 AM
ELF/Config.h
26 ↗(On Diff #35862)

We name this function is64() in COFF.

ELF/Driver.cpp
65–66 ↗(On Diff #35862)

I still think that inlining this function would be better.

70 ↗(On Diff #35862)

Add a new member variable to Config for TargetSource and remove this from this parameter list. I'd name Config->FirstObjName.

82 ↗(On Diff #35862)

*Config->ElfKind doesn't look beautiful. I'd add ELFNoneKind enum and use that to represent the absence of a valid value.

125–143 ↗(On Diff #35862)

This looks more intricate than necessary. Plain if-then-else would be better.

if (Emul == "elf_i386") {
  ...
  return
}
if (Emul == ...) {
}
error("unknown emulation name: "...)
187 ↗(On Diff #35862)

We don't have a comment for other options above.

rafael added inline comments.Sep 28 2015, 1:34 PM
ELF/Driver.cpp
51 ↗(On Diff #35862)

Drop this function. You can use the existing one.

And please rebase the patch:

$ git apply -p0 ~/Downloads/D13055.diff
error: patch failed: ELF/Config.h:28
error: ELF/Config.h: patch does not apply
error: patch failed: ELF/Driver.cpp:47
error: ELF/Driver.cpp: patch does not apply
error: patch failed: ELF/Options.td:49
error: ELF/Options.td: patch does not apply

denis-protivensky marked 8 inline comments as done.

Updated:

  • Rename is64Bits() -> is64()
  • Add FirstObjFile config parameter
  • Introduce ELFNoneKind
  • Replace applyEmulation loop with if-else
  • Remove configureTarget function
  • Drop getElfMachineType function
ruiu added inline comments.Sep 30 2015, 10:53 AM
ELF/Driver.cpp
65–67 ↗(On Diff #36088)

I think you can remove Twine() from this statement. Twine is to concatenate strings, so at least grouping an entire string with Twine() doesn't make sense.

rafael added inline comments.Sep 30 2015, 2:39 PM
ELF/Config.h
30 ↗(On Diff #36088)

git-clang-format

ELF/SymbolTable.cpp
26 ↗(On Diff #36088)

git-clang-format

You are missing a testcase where -m *is* compatible with the rest of
the elf files :-)

Add a new member variable to Config for TargetSource and remove this from this parameter list. I'd name Config->FirstObjName.

The first object name is always available in the SymbolTable:

SymbolTable.getFirstELF()->getName()

Why do you need the extra variable?

You are missing a testcase where -m *is* compatible with the rest of
the elf files :-)

I do have these tests in emulation.s file:

Sorry for the previous not full comment, just hit Submit button accidentally.

I do have these tests in emulation.s file:

# RUN: lld -flavor gnu2 -m elf64ppc %tppc64 -o %t2ppc64
denis-protivensky marked 2 inline comments as done.

Updated:

  • Remove FirstOjbName Config variable, use Symtab.getFirstELF()->getName() instead
  • clang-format sources

This patch makes isCompatibleWith redundant and should remove it (See
attached patch).

I think this OK once rebased on top of trunk.
Rui, is this OK with you too?

Updated:

  • Remove isCompatibleWith method
  • Rename OPT_emulation -> OPT_m
  • Rename applyEmulation -> setELFType

Returned back FirstObjName config variable since we need a flag indicating whether ELF parameters were grabbed from the first object file or from the -m option.

Returned back FirstObjName config variable since we need a flag indicating whether ELF parameters were grabbed from the first object file or from the -m option.

Why?

You report an error as soon as an incompatibility is found. If
getFirstELF returns non null, you know either

  • There was a -m and it is compatible with it
  • There was no -m and you don't have to worry about it

In any case you only have to check if that file is compatible with the
current one.

Cheers,
Rafael

See the attached patch for example.

Just one more nit:

in ELFFileBase::getEMachine() I think you have to keep the
llvm_unreachable outside of the switch to avoid a gcc warning.

LGTM with that.

Updated:

  • Remove FirstObjName in favor of always reporting incompatibility with the first object file if available.

Just one more nit:

in ELFFileBase::getEMachine() I think you have to keep the
llvm_unreachable outside of the switch to avoid a gcc warning.

LGTM with that.

It's clang issuing warning because of extending ELFKind enum with ELFNoneKind value, that's why I moved the check. gcc doesn't give warning in either cases.

ruiu added inline comments.Oct 5 2015, 11:09 AM
ELF/Driver.cpp
96 ↗(On Diff #36512)

I'd avoid giving the same name even if they live in different namespaces. Name this createELFInputFile.

107 ↗(On Diff #36512)

Flip this condition to return early.

108–110 ↗(On Diff #36512)

Separate two conditions

if (ELFFileBase *F = Symtab.getFirstELF())
  error(MB.getBufferIdentifier() + " is not compatible with" + F->getName());
error(MB.getBufferIdentifier() + " is not compatible with target architecture");
109–110 ↗(On Diff #36512)

Can you remove Twine()? The result type of the expression inside Twine() is std::string, and because std::string -> Twine implicit conversion is defined, I believe you don't need that.

denis-protivensky marked 4 inline comments as done.

Updated:

  • Rename createELFFile -> createELFInputFile
  • Rework error handling in createELFInputFile - use early return, split error messages
ruiu accepted this revision.Oct 6 2015, 9:14 AM
ruiu added a reviewer: ruiu.

LGTM

This revision is now accepted and ready to land.Oct 6 2015, 9:14 AM
This revision was automatically updated to reflect the committed changes.