Page MenuHomePhabricator

Add and fix some tests for PPC64
ClosedPublic

Authored by alexandreyy on Mar 14 2018, 8:14 AM.

Details

Summary

TestExprsChar.py
Char is unsigned char by default in PowerPC.

TestDisassembleBreakpoint.py
Modify disassemble testcase to consider multiple architectures.

TestThreadJump.py
Jumping directly to the return line on PowerPC architecture dos not
means returning the value that is seen on the code. The last test fails,
because it needs the execution of some assembly in the beginning of the
function. Avoiding this test for this architecture.

TestEhFrameUnwind.py
Implement func for ppc64le test case.

TestWatchLocation.py
TestStepOverWatchpoint.py
PowerPC currently supports only one H/W watchpoint.

TestDisassembleRawData.py
Add PowerPC opcode and instruction for disassemble testcase.

Diff Detail

Repository
rL LLVM

Event Timeline

alexandreyy created this revision.Mar 14 2018, 8:14 AM
alexandreyy added a subscriber: lldb-commits.

I wonder if we shouldn't just fix the TestDisassembleBreakpoint to not require adding every single architecture. That test was added because lldb-server was not removing the traps from the memory read packets. This is something that is completely independent of us later trying to disassemble the memory we got back. To test that, it should be sufficient to compare the memory contents before and after setting a breakpoint.

Guessing what instructions will the compiler emit for a particular piece of C will just make the test fragile...

packages/Python/lldbsuite/test/make/Makefile.rules
189 ↗(On Diff #138361)

Why do you need both powerpc64le and ppc64le?

Modified TestDisassembleBreakpoint.py to consider multiple architectures.

I wonder if we shouldn't just fix the TestDisassembleBreakpoint to not require adding every single architecture. That test was added because lldb-server was not removing the traps from the memory read packets. This is something that is completely independent of us later trying to disassemble the memory we got back. To test that, it should be sufficient to compare the memory contents before and after setting a breakpoint.

Guessing what instructions will the compiler emit for a particular piece of C will just make the test fragile...

I executed the tests again and we don't really need both powerpc64le and ppc64le.
Some tests required "ppc64le" to be built. But they are working now without it.
I modified the disassemble test as you suggested.

alexandreyy edited the summary of this revision. (Show Details)Mar 15 2018, 1:21 PM
labath accepted this revision.Mar 16 2018, 2:25 AM

I like what you did with the test. Originally, I wanted to just compare the raw memory contents, but this keeps it more inline with the spirit of the original test. I have just one question about the list zipping, but otherwise lgtm.

packages/Python/lldbsuite/test/functionalities/disassembly/TestDisassembleBreakpoint.py
48–49 ↗(On Diff #138610)

what will this do if the lists are of different length? Should you assert that the length matches as well?

This revision is now accepted and ready to land.Mar 16 2018, 2:25 AM

The test would pass if one of the disassembled code had more instructions.
That should be considered.
I added the assertion for that.
Thanks @labath .

I like what you did with the test. Originally, I wanted to just compare the raw memory contents, but this keeps it more inline with the spirit of the original test. I have just one question about the list zipping, but otherwise lgtm.

Hi @labath .
Could you merge this patch?

Leonardo, this breaks the Mac OS X bot.
I'm going to revert this to get the bots green again, unless you have
a super quick fix (please let me know :)

@davide Are you sure this is the correct patch? This one was not merged yet.
Could you give more details about the issue? Is it related with one of our changes?

@davide Are you sure this is the correct patch? This one was not merged yet.
Could you give more details about the issue? Is it related with one of our changes?

Apologies, this is the right one

commit 983ecd497e223b35e46fd3306bd7ac300da94793 (HEAD -> master)
Author: Davide Italiano <ditaliano@apple.com>
Date: Tue Mar 20 11:12:19 2018 -0700

Revert "Fix some tests for PPC64le architecture"

It broke the lldb Mac OSX greendragon bot.

@davide Are you sure this is the correct patch? This one was not merged yet.
Could you give more details about the issue? Is it related with one of our changes?

Apologies, this is the right one

commit 983ecd497e223b35e46fd3306bd7ac300da94793 (HEAD -> master)
Author: Davide Italiano <ditaliano@apple.com>
Date: Tue Mar 20 11:12:19 2018 -0700

Revert "Fix some tests for PPC64le architecture"
 
It broke the lldb Mac OSX greendragon bot.

here are the bot logs (for your convenience)
http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/5869/console

@davide, I think the error is the variable number, as this is the only command in the test case, could you just change from

# Do anonymous symbols work?
self.expect("expression ((char**)environ)[0]",
        startstr="(char *) $1 = 0x")

to

# Do anonymous symbols work?
self.expect("expression ((char**)environ)[0]",
        startstr="(char *) $0 = 0x")

and check if it works?

@davide Let me know if you need that I change it on my diff or any other help.

I think I understand why this is failing:

AssertionError: False is not True : 'expression ((char**)environ)[0]'
returns expected result, got '(char *) $0 = 0x00007ffeefbff753
"COMMAND_MODE=unix2003"'
Config=x86_64-/Users/davide/work/llvm-monorepo/build-release/bin/clang-7.0

but the test shows $1. How did this work for you? Are you running on
Linux by any chance?

Thanks,

Davide

Yes, I'm running Linux and this test was not working for me, so as discussion above, it was separated to be executed on non Linux and Windows, but the variable number got wrong. Sorry for that.

Hi @labath .
Could you please commit this patch?

This revision was automatically updated to reflect the committed changes.