Page MenuHomePhabricator

llvm-objdump:Fix Bugzilla ID 41862 to support checking addresses of disassembled object
ClosedPublic

Authored by xerofoify on May 15 2019, 4:55 PM.

Details

Summary

This fixes the bugzilla id,41862 to support dealing with checking
stop address against start address to support this not being a
proper object to check the disasembly against like gnu objdump
currently does.

Diff Detail

Repository
rL LLVM

Event Timeline

xerofoify created this revision.May 15 2019, 4:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 4:55 PM

When uploading changes make sure to upload the whole context. If using git diff use git diff -U100000 or some other large number to get all of the surrounding context.

Can you also explain why this should be an error? It feels to me like this is perfectly valid and that objdump should just display nothing. At best this exact case should be a warning IMO.

Also what does GNU objdump do in this case?

llvm/tools/llvm-objdump/llvm-objdump.cpp
1379 ↗(On Diff #199702)

If this is in fact the behavior we want we can just make the above operator '>=' and the error message will still be valid (actually as is the error message is technically wrong but would be made right if used '>=')

It fails after looking at the code again for the gnu binutils so it would be best to keep them compatible IMO. However merging my if statement together is better through so I
keep upload a new patch with the git diff comments as I was just using git format patch before. In addition it seems that there is also the case in binutils of lesser
than stop address so that should be added as well so I guess we really do need another if statement. Let me know if those changes work for you to keep compatibility with
gnu binutils.

Thanks for working on this! Please remember to check that the bug is not assigned to somebody when you start working on it, and assign it to yourself when you do.

Test case?

It fails after looking at the code again for the gnu binutils so it would be best to keep them compatible IMO. However merging my if statement together is better through so I
keep upload a new patch with the git diff comments as I was just using git format patch before. In addition it seems that there is also the case in binutils of lesser
than stop address so that should be added as well so I guess we really do need another if statement. Let me know if those changes work for you to keep compatibility with
gnu binutils.

I'm not sure I quite follow your comment here. For error messages, I don't think we need to worry about compatibility with GNU, so it's fine if they're different. I think having a single if is all that is needed. I'd expect the patch to be the following:

if (StartAddress >= StopAddress)
  error("Start address should be less than stop address");

Also what does GNU objdump do in this case?

For reference, I filed the bug based on the GNU behaviour. The bug even says so ;)

xerofoify added a comment.EditedMay 17 2019, 9:22 AM

Yes you did and gnu generally errors here. My point was is there is a third case so the if should really be:
if (StartAddress >= StopAddress)

error("Start address should be less than stop address");

if (StartAddress < StopAddress)

error("Stop address should be greater than start address");

as this was missing and seems to occur in binutils on the gnu side.

And probably switch to lowercase for the first character if you intend to change the error message. The lower case is more common elsewhere. As a reference, the message from GNU objdump is error: the stop address should be after the start address

nit: the title should start with [llvm-objdump]

There's already a test in llvm/test/tools/llvm-objdump/X86/start-stop-address.test. Can you add cases for both stop < start and stop == start there? You should use // RUN: not llvm-objdump -d ... and FileCheck the error message

llvm/tools/llvm-objdump/llvm-objdump.cpp
1379 ↗(On Diff #199702)

Yep, I think we should just have one error. Just changing the previous check from > to >= should work.

xerofoify updated this revision to Diff 201122.May 23 2019, 9:27 PM

This is the updated patch. I wasn't sure if the test cases are correct, not sure if the syntax has issues. Otherwise this should be fixed now and thanks for the comments.

jhenderson requested changes to this revision.May 24 2019, 2:15 AM

This is the updated patch. I wasn't sure if the test cases are correct, not sure if the syntax has issues. Otherwise this should be fixed now and thanks for the comments.

I'm not sure what you mean by "not sure if the syntax has issues"? If it's syntactically invalid, the test won't work, I think (indeed, I don't think it is). Have you run the tests?

llvm/test/tools/llvm-objdump/X86/start-stop-address.test
71 ↗(On Diff #201122)

2=&1 is not correct. You should have 2>&1 - that redirects stderr and merges it to stdout, so that all output and error messages are then passed to FileCheck. The syntax is the same as bash etc here.

73 ↗(On Diff #201122)

1>&2 will redirect stdout to stderr which isn't what you want to do. Also, this behaviour makes no sense.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1408–1409 ↗(On Diff #201122)

Ummm... what?

We now have:
Start address == stop address -> error
Start address < stop address -> error
Start address > stop address -> error

Under what circumstances do we not emit an error? Why do you need the second if here?

This revision now requires changes to proceed.May 24 2019, 2:15 AM
xerofoify updated this revision to Diff 201373.May 24 2019, 5:37 PM

Sorry about the issues. I looked through the comments and it seems that the test cases below are correct and one needed to just be added by copying and pasting the other one but checking for the same addresses with a newer error message to check against. Hopefully this is correct now and thanks for the patience.

rupprecht added inline comments.May 28 2019, 10:02 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1407 ↗(On Diff #201373)

The original error message is correct; the start should be less than the stop. The only issue was we didn't error when start == stop. Also, btw, the error message should automatically error, so adding it to the error string is redundant.

I'm fine with lower casing Start -> start though.

Also minor nit with the patch title: you should start it with [llvm-objdump] so it's easy to parse when looking through a list of commits which components it affects (e.g. see https://reviews.llvm.org/rL361395)

xerofoify retitled this revision from Fix Bugzilla ID 41862 to support checking addresses of disassembled object to llvm-objdump:Fix Bugzilla ID 41862 to support checking addresses of disassembled object.May 28 2019, 10:39 AM
xerofoify marked an inline comment as done.May 28 2019, 10:48 AM

Sorry about that I thought the patch name needed to change just not here. That's fixed and the error message is back to normal with the added condition and test case.

Sorry about that I thought the patch name needed to change just not here. That's fixed and the error message is back to normal with the added condition and test case.

The error message I'm seeing still is wrong. I don't think you've updated the diff. Please do so.

Sorry about that I thought the patch name needed to change just not here. That's fixed and the error message is back to normal with the added condition and test case.

The error message I'm seeing still is wrong. I don't think you've updated the diff. Please do so.

Sorry multiple versions of patch and mislinked wrong one. The correct one is now the current diff after just checking here.

xerofoify updated this revision to Diff 202769.Jun 3 2019, 11:52 AM

Have you run the test since making your change to the error message? I'm guessing you haven't because the case of "Start" in the test does not match that in the code.

xerofoify updated this revision to Diff 203131.Jun 5 2019, 6:31 AM
jhenderson accepted this revision.Jun 6 2019, 2:50 AM

LGTM. Do you need somebody to commit for you?

This revision is now accepted and ready to land.Jun 6 2019, 2:50 AM

Yes as I don't have commit privileges.

This revision was automatically updated to reflect the committed changes.