This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Add warning if --disassemble-functions specifies an unknown symbol
ClosedPublic

Authored by mmpozulp on May 22 2019, 5:06 PM.

Event Timeline

mmpozulp created this revision.May 22 2019, 5:06 PM

I've taken a bit of a heavy-hammer approach here. In order to use set_difference() on StringSet I had to extend the StringMap and StringSet APIs. Are the downsides of having a bigger API warranted by the ability to use set_difference() on StringSet? If so, I can break it out into a different patch. An alternative would be to write this patch without calling set_difference(), but that imposes some code duplication.

mmpozulp edited the summary of this revision. (Show Details)May 22 2019, 5:15 PM

I like what you're doing, and I think having count and insert operations on a StringSet make a lot of sense from a somewhat naive perspective, but I think those should be in a separate change, with unit tests and with reviewers who know that class better than I do. You can cite this review in that one for the motivation (also citing that it makes it more STL-like).

I've added a few more reviewers, partly because I'm away all of next week.

llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test
2

Nit: missing trailing full stop.

11

By the way, the trailing "..." isn't needed in any case I've seen so far for yaml2obj.

13

I think the more common approach is for warnings and errors to start with lower case and not have a trailing full stop.

llvm/tools/llvm-objdump/llvm-objdump.cpp
379

StringRef would be more appropriate here, I think.

1412–1413

I think it would be clearer here if you don't use auto.

1415

No need for the const & for a StringRef.

mmpozulp updated this revision to Diff 201139.May 24 2019, 1:15 AM

Incorporate feedback from @jhenderson, including moving ADT changes (which must land before this patch) to a separate patch https://reviews.llvm.org/D62369

jhenderson accepted this revision.May 24 2019, 2:46 AM

Looks good, apart from one small question. I'm away after the end of the day, so you'll need to get someone else to land this for you. Alternatively, request commit access as documented here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

FYI, you can also mark patches as requiring committing before or after other patches by using the "edit related objects" button.

llvm/tools/llvm-objdump/llvm-objdump.cpp
381

Why are you flushing here?

This revision is now accepted and ready to land.May 24 2019, 2:46 AM
mmpozulp updated this revision to Diff 201343.May 24 2019, 3:06 PM

Incorporate feedback from @jhenderson to remove unneeded errs().flush().

jhenderson accepted this revision.Jun 3 2019, 3:44 AM

Thanks, LGTM. Did you already commit this? If so, could you post a link to the revision that you committed? If not, so that you know, you didn't need to wait for another LGTM, since I already gave you one which you addressed.

MaskRay added inline comments.Jun 4 2019, 6:40 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1414

The if (!MissingDisasmFuncsSet.empty()) is redundant.

rupprecht accepted this revision.Jun 6 2019, 10:30 AM

LGTM except for MaskRay's comment about the unnecessary if check

I'm fighting with svn. See my comment in https://reviews.llvm.org/D62369.

mmpozulp updated this revision to Diff 203491.Jun 6 2019, 9:22 PM

Incorporate feedback from @MaskRay to delete redundant if-statement.

MaskRay accepted this revision.Jun 6 2019, 9:24 PM
This revision was automatically updated to reflect the committed changes.

Committed as r362768. Thanks!

mmpozulp reopened this revision.Jun 7 2019, 1:27 PM
This revision is now accepted and ready to land.Jun 7 2019, 1:27 PM
mmpozulp edited the summary of this revision. (Show Details)Jun 7 2019, 1:28 PM

Committed as r362838.

This revision was automatically updated to reflect the committed changes.