This is an archive of the discontinued LLVM Phabricator instance.

[ELF][MIPS] Handle mips in the OUTPUT_FORMAT directive
ClosedPublic

Authored by atanasyan on Nov 26 2018, 3:10 PM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

atanasyan created this revision.Nov 26 2018, 3:10 PM
ruiu added inline comments.Nov 26 2018, 4:40 PM
ELF/ScriptParser.cpp
430

I don't think returning a bfd name makes sense. You should simply return a boolean value indicating whether it is MIPS N32 ABI or not.

But why MIPS always does too many things slightly differently? Do you really need to support both tradmips and ntradmips?

  • Return a boolean value indicating whether the name is MIPS N32 ABI or not from the readBfdName .

I think one of the problem is that there are multiple really different architectures and ABIs under the same EM_MIPS name. In particular, maybe it was better to introduce two different names EM_MIPS and EM_MIPS_64 many years ago. In that case ntradmips (N32 ABI) could be describes like {ELF32LEKind, EM_MIPS_64}.

grimar added inline comments.Nov 27 2018, 1:07 AM
ELF/ScriptParser.cpp
388

I would suggest doing something like:

namespace {
  struct BfdArchKind {
    ELFKind BfdEKind;
    uint16_t BfdEMachine;
    bool IsMipsN32Abi;
  };
};

static BfdArchKind parseBfdName(StringRef Name) {
...

i.e. use a struct instead of std::tuple and a static helper instead of the member function.

grimar added inline comments.Nov 27 2018, 1:09 AM
ELF/ScriptParser.cpp
388

It seems a bit more readable because it is sometimes unobvious what pair/tuple do contain.
Here the third boolean argument made me wonder what is contained there.
Using a struct with named members can help.

atanasyan marked an inline comment as done.Nov 27 2018, 2:06 AM
atanasyan added inline comments.
ELF/ScriptParser.cpp
388

I like an idea to use a helper structure, but unfortunately we cannot make parseBfdName a static helper because it calls private ScriptParser::next().

ruiu added a comment.Nov 27 2018, 9:29 AM
  • Return a boolean value indicating whether the name is MIPS N32 ABI or not from the readBfdName .

I think one of the problem is that there are multiple really different architectures and ABIs under the same EM_MIPS name. In particular, maybe it was better to introduce two different names EM_MIPS and EM_MIPS_64 many years ago. In that case ntradmips (N32 ABI) could be describes like {ELF32LEKind, EM_MIPS_64}.

I agree with that. Too many different things are categorized under the same name just because it is considered just "MIPS". That's unfortunate.

ELF/ScriptParser.cpp
428–435

This is a bit shorter:

std::tuple<ELFKind, uint16_t, bool> Tuple = readBfdName();
if (Config->EKind == ELFNoneKind)
  std::tie(Config->EKind, Config->EMachine, COnfig->MipsN32ABI) = Tuple;
ruiu added a comment.Nov 27 2018, 9:29 AM

As to defining a struct to store the parse result of parseBfdName, I don't think we need to define a new struct. That seems a bit too heavyweight to represent just a temporary value.

  • Simplify handling of the readBfdName return value
grimar accepted this revision.Nov 28 2018, 3:16 AM

LGTM.

This revision is now accepted and ready to land.Nov 28 2018, 3:16 AM
This revision was automatically updated to reflect the committed changes.