This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Move --start-address >= --stop-address check out of the -d code.
ClosedPublic

Authored by ychen on Jun 20 2019, 4:52 PM.

Details

Summary

Move it into main function so the checking is effective for all actions user may do with llvm-objdump; notably, -r and -s in
addition to existing -d. Match GNU behavior.

prepare for patch on PR41911

Diff Detail

Event Timeline

ychen created this revision.Jun 20 2019, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 4:52 PM

I think the description should mention that the --start-address >= --stop-address check has been moved out of the -d code. This can be useful for -t -r

ychen retitled this revision from [llvm-objdump] Perform --start-address / --stop-address error checking to [llvm-objdump] Move --start-address >= --stop-address check out of the -d code..Jun 20 2019, 8:15 PM
ychen edited the summary of this revision. (Show Details)

Thank you @MaskRay. Description changed. I intentionally leave out -t since I suspect it is a bug. See my comment in https://bugs.llvm.org/show_bug.cgi?id=41911.

MaskRay added inline comments.Jun 20 2019, 8:27 PM
llvm/test/tools/llvm-objdump/X86/start-stop-address.test
71

If this line (no command specified) gives diagnostics, the -d -r -s RUN lines may be unnecessary.

ychen updated this revision to Diff 205940.Jun 20 2019, 8:49 PM
  • update
ychen marked an inline comment as done.Jun 20 2019, 8:49 PM
ychen marked an inline comment as done.Jun 20 2019, 8:50 PM
ychen added inline comments.
llvm/test/tools/llvm-objdump/X86/start-stop-address.test
71

Missed that.

MaskRay accepted this revision.Jun 20 2019, 9:37 PM
This revision is now accepted and ready to land.Jun 20 2019, 9:37 PM
grimar added inline comments.Jun 21 2019, 1:26 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
2117

Does it make sence to include the address values here? And/or the option name?
It is not really clear from the error message text what is wrong it seems.

grimar accepted this revision.Jun 21 2019, 1:27 AM

My comment is not directly related to change done in this patch though, so LGTM.

jhenderson accepted this revision.Jun 21 2019, 1:33 AM
jhenderson added inline comments.
llvm/test/tools/llvm-objdump/X86/start-stop-address.test
70–72

I think this change should only remove the -d part, not change the ordering of the --stop-address values.

llvm/tools/llvm-objdump/llvm-objdump.cpp
2117

I agree that it might be nice to improve the error message. I'm okay with that being a separate change though.

This revision was automatically updated to reflect the committed changes.
ychen marked 4 inline comments as done.Jun 21 2019, 6:21 PM
ychen added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
2117

I'll leave it in another patch. Thank you!