This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] - Move getRelocationValueString and dependenices out of the llvm-objdump.cpp
ClosedPublic

Authored by grimar on Jan 17 2019, 3:31 AM.

Details

Summary

getRelocationValueString is a dispatcher function that calls the
corresponding ELF/COFF/Wasm/MachO implementations
that currently live in the llvm-objdump.cpp file.

I think these implementations should be moved to ELFDump.cpp,
COFFDump.cpp and other corresponding files, to move platform specific
implementation out from the common logic.

The patch does that. Also, I had to move ToolSectionFilter helper and SectionFilterIterator,
SectionFilter to a header to make them available across the objdump code.
That was needed for COFF, other implementations do not use ToolSectionFilter.
But it should be OK.

It depends on D56721 that was LGTMed but not yet landed.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jan 17 2019, 3:31 AM

This patch only moves the code and renames the few methods. No other changes were performed.

grimar edited the summary of this revision. (Show Details)Jan 17 2019, 3:34 AM
jhenderson accepted this revision.Jan 17 2019, 4:44 AM

It depends on D56721 that was LGTMed but not yet landed.

FYI, I'm just verifying the latest version of this, following the last batch of updates, and I'll commit it after it's passed the test.

LGTM. I agree with your reasoning for moving them.

tools/llvm-objdump/llvm-objdump.h
74 ↗(On Diff #182240)

This is probably fine as is, but I wonder if it actually belongs in the Object library?

This revision is now accepted and ready to land.Jan 17 2019, 4:44 AM
grimar marked an inline comment as done.Jan 17 2019, 5:02 AM
grimar added inline comments.
tools/llvm-objdump/llvm-objdump.h
74 ↗(On Diff #182240)

I would be happy to not have it here. But I think we need to find more users for this to move it out first? Until it is proved to be useful outside of llvm-objdump, I would keep it here.

rupprecht accepted this revision.Jan 17 2019, 10:57 AM
sbc100 added inline comments.Jan 17 2019, 5:24 PM
tools/llvm-objdump/WasmDump.cpp
53 ↗(On Diff #182240)

Missing newline at EOF

tools/llvm-objdump/llvm-objdump.h
139 ↗(On Diff #182240)

Why not continue to use overloads?

grimar marked an inline comment as done.Jan 18 2019, 1:45 AM
grimar added inline comments.
tools/llvm-objdump/llvm-objdump.h
139 ↗(On Diff #182240)

There are few benefits from not using them here, and I see no profit.

Now we have a code that looks like:

static std::error_code getRelocationValueString(const ELFObjectFileBase *Obj,
                                                const RelocationRef &Rel,
                                                SmallVectorImpl<char> &Result) {
  if (auto *ELF32LE = dyn_cast<ELF32LEObjectFile>(Obj))
    return getRelocationValueString(ELF32LE, Rel, Result);
  if (auto *ELF64LE = dyn_cast<ELF64LEObjectFile>(Obj))
    return getRelocationValueString(ELF64LE, Rel, Result);
  if (auto *ELF32BE = dyn_cast<ELF32BEObjectFile>(Obj))
    return getRelocationValueString(ELF32BE, Rel, Result);
  auto *ELF64BE = cast<ELF64BEObjectFile>(Obj);
  return getRelocationValueString(ELF64BE, Rel, Result);
}

static std::error_code getRelocationValueString(const RelocationRef &Rel,
                                                SmallVectorImpl<char> &Result) {
  const ObjectFile *Obj = Rel.getObject();
  if (auto *ELF = dyn_cast<ELFObjectFileBase>(Obj))
    return getRelocationValueString(ELF, Rel, Result);
  if (auto *COFF = dyn_cast<COFFObjectFile>(Obj))
    return getRelocationValueString(COFF, Rel, Result);
  if (auto *Wasm = dyn_cast<WasmObjectFile>(Obj))
    return getRelocationValueString(Wasm, Rel, Result);
  if (auto *MachO = dyn_cast<MachOObjectFile>(Obj))
    return getRelocationValueString(MachO, Rel, Result);
  llvm_unreachable("unknown object file format");
}

It seems to be completely unreadable. Looking at this code I can hardly realize that dispatcher function getRelocationValueString
will call ELF version of getRelocationValueString which will call one more getRelocationValueString basing on ELFT.
It is painful to trace.

The second benefit is that other methods in this header contain the COFF/ELF/Wasm/MachO prefixes/suffixes.
I think we might want to regroup them by the target, having them mixed does not look good.
So renaming will be important for consistency.

These implementations have no much common inside, after all, to name them in the same way. Logically this is a 4 really different implementations.

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.