Page MenuHomePhabricator

[llvm-objdump] Make --disassemble-functions imply -d
ClosedPublic

Authored by mmpozulp on May 16 2019, 11:39 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mmpozulp created this revision.May 16 2019, 11:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 11:39 PM
jhenderson added inline comments.May 17 2019, 3:22 AM
llvm/test/tools/llvm-objdump/X86/disassemble-implied-by-disassemble-functions.test
6 ↗(On Diff #199981)

Nit: I'm trying to fight against unnecessary whitespace in these blobs. Reduce the space between the tag and value to the minimum needed for the values to line up within the block i.e:

  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
​  Machine: EM_X86_64
14 ↗(On Diff #199981)

Get rid of this, as it's not necessary for the test.

15 ↗(On Diff #199981)

Let's reduce this right down to just a single nop instruction (0x90) or ret (0xc3).

20–24 ↗(On Diff #199981)

You probably only need one symbol and section for this test case. The rest of the disassembly/disassemble-functions functionality (such as --disassemble-functions dumps only the specified symbols) should be tested separately (I actually am currently working on those tests, and hope to have a patch up ready by the end of the day).

33 ↗(On Diff #199981)

I don't think you need to check this first bit.

38–42 ↗(On Diff #199981)

You can probably afford to get away with just checking that the add1: and main: lines.

mmpozulp updated this revision to Diff 200163.May 18 2019, 5:42 PM

Incorporate feedback from @jhenderson.

mmpozulp marked an inline comment as done.May 18 2019, 5:48 PM
mmpozulp added inline comments.
llvm/test/tools/llvm-objdump/X86/disassemble-implied-by-disassemble-functions.test
6 ↗(On Diff #199981)

Should we modify obj2yaml to eliminate unnecessary whitespace? I'm an ELF noob so when I made this test case I started with a source (.c) file, compiled to an object (.o) file, and finally ran obj2yaml to generate the yaml file, which seems to always include extra whitespace.

jhenderson accepted this revision.Mon, May 20, 4:02 AM

LGTM, with one minor comment outstanding.

Do you need somebody to commit it for you?

llvm/test/tools/llvm-objdump/X86/disassemble-implied-by-disassemble-functions.test
6 ↗(On Diff #199981)

It might be good to do so. I don't know the background behind why it does that. For the record, I quite often copy an existing yaml blob and work from there. You may find that useful going forwards.

22 ↗(On Diff #200163)

You can use CHECK-NEXT here.

This revision is now accepted and ready to land.Mon, May 20, 4:02 AM
mmpozulp updated this revision to Diff 200400.Mon, May 20, 9:49 PM

Change a CHECK to CHECK-NEXT, thanks @jhenderson.

LGTM, with one minor comment outstanding.

Do you need somebody to commit it for you?

Yes, please. Thanks for your help.

Thanks @mmpozulp! I'll try to commit this later, but it might not happen until tomorrow. I've subscribed some others who have worked on the binutils, who might have time to land it before then.

Thanks @mmpozulp! I'll try to commit this later, but it might not happen until tomorrow. I've subscribed some others who have worked on the binutils, who might have time to land it before then.

I'll do that, NP.

This revision was automatically updated to reflect the committed changes.