This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj, libSupport] - Refine the implementation of the code that dumps build attributes.
ClosedPublic

Authored by grimar on Nov 30 2020, 6:16 AM.

Details

Summary

This implementation of ELFDumper<ELFT>::printAttributes() in llvm-readobj has issues:

  1. It crashes when the content of the attribute section is empty.
  2. It uses unwrapOrError and reportWarning calls, though ideally we want to use reportUniqueWarning.
  3. It contains a TODO about redundant format version check.

lib/Support/ELFAttributeParser.cpp uses a hardcoded constant instead of the named constant.

This patch fixes all these issues.

Diff Detail

Event Timeline

grimar created this revision.Nov 30 2020, 6:16 AM
grimar requested review of this revision.Nov 30 2020, 6:16 AM
grimar updated this revision to Diff 308593.Dec 1 2020, 2:22 AM
grimar edited the summary of this revision. (Show Details)
  • Rebased. Used the new overload of reportUniqueWarning
MaskRay accepted this revision.Dec 1 2020, 2:32 PM
This revision is now accepted and ready to land.Dec 1 2020, 2:32 PM
jhenderson added inline comments.Dec 2 2020, 12:26 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
2907

I'm not sure you need the "the content of" bit of this message. The section itself is the thing that is empty/not empty.

2918–2928

I think you can avoid this dance using the Error as output parameter idiom. See ErrorAsOutParameter.

grimar updated this revision to Diff 308901.Dec 2 2020, 12:55 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
2918–2928

I am not sure I understood tthe idea. Are you saying that parse should accept an Error argument?
ErrorAsOutParameter clears the checked bit in destructor. I wonder how it might help here.

I've replaced consumeError with cantFail.

jhenderson added inline comments.Dec 2 2020, 1:09 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
2918–2928

Never mind, I misread the code. However, I have an alternative idea now: factor the if (Machine == EM_ARM) ... if statement and its corresponding else into a separat function/lambda, then return the Error so that you can do:

Error E = parseAttributes(...);
if (E)
  reportUniqueWarning(...);

What do you think?

grimar updated this revision to Diff 308909.Dec 2 2020, 1:32 AM
grimar marked an inline comment as done.
  • Addressed review comment.
grimar added inline comments.Dec 2 2020, 1:33 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
2918–2928

Done.

jhenderson accepted this revision.Dec 2 2020, 1:39 AM

LGTM, with one nit.

llvm/tools/llvm-readobj/ELFDumper.cpp
2918–2927

Do you need the trailing return type? I don't think there's any conversion involved, so it should work without it?

grimar marked an inline comment as done.Dec 2 2020, 1:41 AM
grimar added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
2918–2927

I think so. I'll remove.

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