Supports common prologue/epilogue instructions.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Looks good if you are not concerned with my inline comment about using bitfields and endianess
source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp | ||
---|---|---|
257–263 ↗ | (On Diff #134464) | 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". |
291–297 ↗ | (On Diff #134464) | Ditto |
362–369 ↗ | (On Diff #134464) | ditto |
401–406 ↗ | (On Diff #134464) | ditto |
source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp | ||
---|---|---|
257–263 ↗ | (On Diff #134464) | Right, in the case you mentioned it will probably not work correctly. |
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 | ||
---|---|---|
257–263 ↗ | (On Diff #134464) | 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 | ||
20 ↗ | (On Diff #134464) | 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. |
79 ↗ | (On Diff #134464) | struct Opcode { |
82 ↗ | (On Diff #134464) | 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 | ||
---|---|---|
20 ↗ | (On Diff #134464) | Yes, putting it in either lldb_private or in anonymous namespace would avoid polluting the global namespace. |
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).
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 | ||
---|---|---|
20 ↗ | (On Diff #134464) | Now that you're inside the namespace, you can remove all the lldb_private:: references below... |
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?
source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h | ||
---|---|---|
85 ↗ | (On Diff #134950) | 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. |
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.