This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Initial version of PPC64 InstEmulation
ClosedPublic

Authored by luporl on Feb 15 2018, 11:13 AM.

Event Timeline

luporl created this revision.Feb 15 2018, 11:13 AM
clayborg accepted this revision.Feb 15 2018, 11:43 AM

Looks good if you are not concerned with my inline comment about using bitfields and endianess

source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
258–264

Will this work correctly on all systems? Bitfields can be tricky with endianness if you are debugging big endian from a little endian built LLDB or vice versa. Might be worth just extracting bits using Bits32 and Bits64 from "Plugins/Process/Utility/InstructionUtils.h".

292–298

Ditto

363–370

ditto

402–407

ditto

This revision is now accepted and ready to land.Feb 15 2018, 11:43 AM
luporl added inline comments.Feb 15 2018, 11:50 AM
source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
258–264

Right, in the case you mentioned it will probably not work correctly.
I'll change this part.

clayborg requested changes to this revision.Feb 15 2018, 11:52 AM

Ok, marking as needs changes.

This revision now requires changes to proceed.Feb 15 2018, 11:52 AM

Some nits, but nothing major from me.

However, since you've recently had to go through writing an emulation test, I'd like to ask you for your preference. Do you prefer this gtest-style test or a FileCheck-style test?

E.g. a FileCheck-style test for this could look like (this is taken from an x86 emulation test but it should be obvious how to apply this to ppc64):

# RUN: llvm-mc -target x86_64-apple-macosx %s | lldb-test unwind --emulate - | FileCheck %s
.text:
  pushq %rbp
# CHECK: 0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
 movq %rsp, %rbp
# CHECK: 1: CFA=rsp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
  xorl %eax, %eax
# CHECK: 4: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
 popq %rbp
# CHECK: 7: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
 retq
source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
258–264

Also, the correct way to handle things like this is to memcpy the value instead of casting the pointer.

source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
21

Should we put the plugin into the lldb_private namespace? Some plugins do that, and some don't, but I was under the impression that putting them into lldb_private is the new way of doing things.

80

struct Opcode {

83

Putting const on a by-value argument is just unnecessary verbosity.

This FileCheck-style test seems to be easier to write and understand.
In fact, I wrote a gtest-style test because:
1- I didn't know I could write a FileCheck-style test instead.
2- This was inspired by the ARM64 plugin, that seemed like the simplest one to understand and follow.

Let me try to convert it to a FileCheck-style test.

source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
21

Yes, putting it in either lldb_private or in anonymous namespace would avoid polluting the global namespace.
I'll put it in lldb_private, as you've said other plugins are already doing it this way now.

labath added a subscriber: davide.Feb 16 2018, 3:34 AM

This FileCheck-style test seems to be easier to write and understand.
In fact, I wrote a gtest-style test because:
1- I didn't know I could write a FileCheck-style test instead.
2- This was inspired by the ARM64 plugin, that seemed like the simplest one to understand and follow.

Let me try to convert it to a FileCheck-style test.

Unfortunately, you can't write a test like that yet. That syntax right now exists only in my imagination, and I was just surveying you to see if there is interest for such a feature. :(

So right now, we will have to stick with the gtest test, but maybe we will convert the test when we get the ability to test it that way (although if you feel like trying to build such a thing (it shouldn't be that hard even), I would be all for it).

luporl updated this revision to Diff 134633.Feb 16 2018, 8:31 AM
  • Merge branch 'master' into ppc64-inst-emu2
  • Addressed review comments
luporl marked 10 inline comments as done.Feb 16 2018, 8:33 AM
clayborg accepted this revision.Feb 16 2018, 8:38 AM
This revision is now accepted and ready to land.Feb 16 2018, 8:38 AM

How would you feel about replacing the custom debug printfs with standard logging messages?

That way, you can always have them available without recompiling (and it's always nice to have more uniformity).

source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
21

Now that you're inside the namespace, you can remove all the lldb_private:: references below...

How would you feel about replacing the custom debug printfs with standard logging messages?

That way, you can always have them available without recompiling (and it's always nice to have more uniformity).

Ok. That was my initial idea, but I was not sure about which category to log the messages to, and looking into the other instruction emulation plugins I found no log messages, so I ended up leaving only debug printfs.

I guess the best category would be LIBLLDB_LOG_UNWIND, right?

How would you feel about replacing the custom debug printfs with standard logging messages?

That way, you can always have them available without recompiling (and it's always nice to have more uniformity).

Ok. That was my initial idea, but I was not sure about which category to log the messages to, and looking into the other instruction emulation plugins I found no log messages, so I ended up leaving only debug printfs.

I guess the best category would be LIBLLDB_LOG_UNWIND, right?

Yeah, that seems to be the best fit here

luporl updated this revision to Diff 134950.Feb 19 2018, 10:50 AM
  • Changed debug printfs to log messages
luporl marked an inline comment as done.Feb 19 2018, 10:51 AM
labath added inline comments.Feb 20 2018, 2:18 AM
source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
86

You shouldn't be storing the Log pointer as a member variable (in a long-lived object at least). The reason for that is that some can do "log enable lldb unwind" half way through the debug session, and expect that logging starts working. However, here the Log variable is initialized when the plugin is created (which is when the process is launched), so if logging is not enabled then, it will never start working in the future.

luporl updated this revision to Diff 135071.Feb 20 2018, 8:55 AM
  • Changed the plugin to not store the Log pointer
luporl marked an inline comment as done.Feb 20 2018, 8:57 AM
labath accepted this revision.Feb 21 2018, 7:28 AM

Great, thanks for iterating on this.

Thanks @labath. Can you check this in for me?

This revision was automatically updated to reflect the committed changes.

Committed as r326224.

I've had to rearrange the tests a bit, as llvm does not like compiling just a part of the files in a directory. So now we have unittests/UnwindAssembly/ARM64 and /PPC64.