This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/llvm-readelf] - Don't fail to dump the object if .dynsym has broken sh_link field.
ClosedPublic

Authored by grimar on Jun 10 2019, 4:45 AM.

Details

Diff Detail

Event Timeline

grimar planned changes to this revision.Jun 10 2019, 4:45 AM
grimar created this revision.
grimar updated this revision to Diff 203803.Jun 10 2019, 4:50 AM
jhenderson added inline comments.Jun 10 2019, 5:43 AM
tools/llvm-readobj/ELFDumper.cpp
1426

I'm a little uncomfortable with this interface. unwrapOrWarn implies that you'll get a warning or an object, yet you actually end up with both, with the latter being default constructed. However, not all objects are default constructible, meaning that this function can't be called for those types. Furthermore, the default type must be something that can be handled further on in the code correctly (imagine how easy it would be for a crash if the return type was a pointer). I think that way lies the risk of bugs. I'd suggest following the more traditional Expected behaviour, something like this:

Type T;
if (Expected<Type> E = getAType())
  T = *E;
else {
  reportWarning("some message);
  T = Type(A, B);
}

In a StringRef case, what you're doing is fine, as long as an empty StringRef makes sense further down, but that's not always the case.

tools/llvm-readobj/llvm-readobj.h
47

Blank line before the function, please.

EO doesn't make sense as the variable name. What does it stand for?

grimar updated this revision to Diff 203812.Jun 10 2019, 6:17 AM
grimar marked an inline comment as done.
  • Addressed review comments.
tools/llvm-readobj/llvm-readobj.h
47

This method was removed completely.

Blank line before the function, please.

This probably was fine, it was consistent with other "unwrapOr*" helpers above. Sometimes in LLVM functions are grouped together without blank lines in between.

EO doesn't make sense as the variable name. What does it stand for?

Copy paste.. Looking on the code above, initially it came from ErrorOr, but then was reused for Expected<T>

jhenderson accepted this revision.Jun 10 2019, 6:52 AM

LGTM, with one remaining suggestion.

tools/llvm-readobj/llvm-readobj.h
26

I'd call this function warn, not warning (warning is not a verb).

This revision is now accepted and ready to land.Jun 10 2019, 6:52 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 7:20 AM