This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwp] Refuse DWARFv5 input DWP files.
ClosedPublic

Authored by ikudrin on Mar 31 2020, 7:25 AM.

Details

Summary

The library can parse DWARFv5 unit index sections of DWP files, but llvm-dwp is not ready to process them. Refuse such input files for now.

Diff Detail

Event Timeline

ikudrin created this revision.Mar 31 2020, 7:25 AM
grimar added a comment.Apr 1 2020, 1:59 AM

(I know little about llvm-dwp, but have 2 minor general comments)

llvm/tools/llvm-dwp/llvm-dwp.cpp
616

I'd suggest to include the unsupported version number to new error messages.

e.g: "unsupported cu_index version: 99"

656

Is it tested somewhere? I see you've added a test for "cu_index", but this line is about "tu_index".

ikudrin marked an inline comment as done.Apr 1 2020, 8:26 AM
ikudrin added inline comments.
llvm/tools/llvm-dwp/llvm-dwp.cpp
656

Well, not. This check is added for completeness. To get here, the DWP file should have a "cu_index" section of version 2 and a "tu_index" section of version 5, which seems weird. Or should I add such an odd test anyway?

dblaikie added inline comments.Apr 1 2020, 2:17 PM
llvm/tools/llvm-dwp/llvm-dwp.cpp
656

Testing error cases is all about weird - yeah, would be good to have such a test case.

grimar added inline comments.Apr 2 2020, 1:32 AM
llvm/tools/llvm-dwp/llvm-dwp.cpp
656

+1.

ikudrin updated this revision to Diff 254469.Apr 2 2020, 3:13 AM
ikudrin marked 4 inline comments as done.
  • Added reporting the version of an unsupported index.
  • Added a test for the unsupported TU index version case.
  • Removed the actual CU in the test for the CU index version because it is not necessary.
dblaikie accepted this revision.Apr 24 2020, 3:03 PM

Looks good - thanks!

llvm/tools/llvm-dwp/llvm-dwp.cpp
657

I think if you make this first thing a StringRef or Twine, rather than a std::string, you should be able to concatenate the rest without needing to wrap the last literal in a std::string, eg:

StringRef("foo")  + utostr(x) + "bar"
This revision is now accepted and ready to land.Apr 24 2020, 3:03 PM
ikudrin marked 2 inline comments as done.Apr 25 2020, 4:58 AM
ikudrin added inline comments.
llvm/tools/llvm-dwp/llvm-dwp.cpp
657

Thanks! I've applied not exactly your variant, but since utostr() returns std::string, the wrappings of the first and the last string are not necessary at all.

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