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

[xerofoify]

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.

[jakehehrlich]

link to the bug for the lazy: https://bugs.llvm.org/show_bug.cgi?id=41862

[jakehehrlich]

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.

[jakehehrlich]

Also what does GNU objdump do in this case?

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

1379

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 '>=')

[xerofoify]

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.

[jhenderson]

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?

In D61969#1505389, @xerofoify wrote:

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");

[jhenderson]

In D61969#1505356, @jakehehrlich wrote:

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]

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.

[MaskRay]

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

[rupprecht]

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

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

[xerofoify]

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]

In D61969#1515166, @xerofoify wrote:

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

72

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.

74

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

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?

[xerofoify]

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]

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

1407

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.

[rupprecht]

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]

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.

[jhenderson]

In D61969#1519537, @xerofoify wrote:

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.

[xerofoify]

In D61969#1527349, @jhenderson wrote:

In D61969#1519537, @xerofoify wrote:

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.

[jhenderson]

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.

[jhenderson]

LGTM. Do you need somebody to commit for you?

[xerofoify]

Yes as I don't have commit privileges.

llvm/test/tools/llvm-objdump/X86/start-stop-address.test

 // STOP-EMPTY:
 // STOP-NEXT:  0000000000000010 main:
 // STOP-NEXT:      10: 55                            pushq   %rbp
 // STOP-NOT:       {{.}}
 
 // OUT-OF-RANGE-NOT: Disassembly
 
 // RUN: not llvm-objdump -d %t.out --start-address=0x40 --stop-address=0x3f 2>&1 | FileCheck %s --check-prefix ERRMSG
+// RUN: not llvm-objdump -d %t.out --start-address=0x40 --stop-address=0x40 2>&1 | FileCheck %s --check-prefix ERRMSG
 // ERRMSG: error: Start address should be less than stop address.

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

#endif
           // Print relocation for instruction.
           while (RelCur != RelEnd) {
             uint64_t Offset = RelCur->getOffset();
             // If this relocation is hidden, skip it.
             if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {
               ++RelCur;
               continue;
             }
 
             // Stop when RelCur's offset is past the current instruction.
             if (Offset >= Index + Size)
               break;
 
             // When --adjust-vma is used, update the address printed.
             if (RelCur->getSymbol() != Obj->symbol_end()) {
               Expected<section_iterator> SymSI =
                   RelCur->getSymbol()->getSection();
#endif
 
         Index += Size;
       }
     }
   }
 }
 
 static void disassembleObject(const ObjectFile *Obj, bool InlineRelocs) {
-  if (StartAddress > StopAddress)
-    error("Start address should be less than stop address");
+  if (StartAddress >= StopAddress)
+    error("start address should be less than stop address");
 
   const Target *TheTarget = getTarget(Obj);
 
   // Package up features to be passed to target/subtarget
   SubtargetFeatures Features = Obj->getFeatures();
   if (!MAttrs.empty())
     for (unsigned I = 0; I != MAttrs.size(); ++I)
       Features.AddFeature(MAttrs[I]);
 
   std::unique_ptr<const MCRegisterInfo> MRI(