Page MenuHomePhabricator

[llvm-readelf/obj] - Move unique warning handling logic to the `ObjDumper`.
ClosedPublic

Authored by grimar on Nov 27 2020, 2:51 AM.

Details

Summary

This moves the reportUniqueWarning method to the base class.

My motivation is the following:
I've experimented with replacing reportWarning calls with reportUniqueWarning
in ELF dumper. I've found that for example for removing them from DynRegionInfo helper
class, it is worth to pass a dumper instance to it (to be able to call dumper()->reportUniqueWarning()).
The problem was that ELFDumper<ELFT> is a template class. I had to make DynRegionInfo to be templated
and do lots of minor changes everywhere what did not look reasonable/nice.

At the same time I guess one day other dumpers like COFF/MachO/Wasm etc might want to
start using reportUniqueWarning API too. Then it looks reasonable to move the logic to the
base class.

With that the problem of passing the dumper instance will be gone.

Diff Detail

Event Timeline

grimar created this revision.Nov 27 2020, 2:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 2:51 AM
grimar requested review of this revision.Nov 27 2020, 2:51 AM
jhenderson accepted this revision.Nov 27 2020, 2:56 AM

LGTM. The inline suggestion probably deserves a separate patch. Might want to wait and give others an opportunity to chime in, especially as it's Thanksgiving weekend in the US.

llvm/tools/llvm-readobj/ObjDumper.cpp
43–44

Maybe we could simplify this by having WarningHandler not return an Error at all? What do you think?

This revision is now accepted and ready to land.Nov 27 2020, 2:56 AM
grimar marked an inline comment as done.Nov 30 2020, 11:57 PM
grimar added inline comments.
llvm/tools/llvm-readobj/ObjDumper.cpp
43–44

I'll take a look.

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.
grimar added inline comments.Dec 1 2020, 12:38 AM
llvm/tools/llvm-readobj/ObjDumper.cpp
43–44

I think we can't do this, because this WarningHandler is used as argument to the API from ELF.h:

// This is a callback that can be passed to a number of functions.
// It can be used to ignore non-critical errors (warnings), which is
// useful for dumpers, like llvm-readobj.
// It accepts a warning message string and returns a success
// when the warning should be ignored or an error otherwise.
using WarningHandler = llvm::function_ref<Error(const Twine &Msg)>;
....
template <class ELFT>
Expected<StringRef>
ELFFile<ELFT>::getSectionName(const Elf_Shdr &Section,
                              WarningHandler WarnHandler) const {
  auto SectionsOrErr = sections();
  if (!SectionsOrErr)
    return SectionsOrErr.takeError();
  auto Table = getSectionStringTable(*SectionsOrErr, WarnHandler);
  if (!Table)
    return Table.takeError();
  return getSectionName(Section, *Table);
}