This is an archive of the discontinued LLVM Phabricator instance.

Fix some tests for PPC64le architecture
ClosedPublic

Authored by lbianc on Mar 5 2018, 11:20 AM.

Details

Summary
  • Fix test jump for powerpc64le Jumping directly to the return line on power 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.
  • Avoid evaluate environ variable name on Linux On Linux the Symbol environ conflicts with another variable, then in order to avoid it, this test was moved into a specific test, which is not supported if the OS is Linux.
  • Added PPC64le as MIPS behavior Checking the disassembler output, on PPC64le machines behaves as MPIS. Added method to identify PPC64le architecture and checking it when disassembling instructions in the test case.

Event Timeline

lbianc created this revision.Mar 5 2018, 11:20 AM
labath added a comment.Mar 6 2018, 2:40 AM

The __environ trick may work on other *linux* architectures, but it does not work everywhere (I've just checked mac). I bet the other conflicting symbol you see is in the dynamic loader (you can check with "image lookup -s environ"). The failure is not ppc-specific -- it fails on all linux machines which have a symbol table for the dynamc linker. This is fixable by teaching lldb about the dynamic loader module on linux, which will cause it to not look for symbols there by default (because the normal symbol lookup within the inferior will not look there either). This is why this test passes on mac, but implementing this for linux is not as simple as mac -- there we just check the file header, but on linux there is nothing about the file itself that would tell us that this is really a dynamic linker (at least i haven't found anything). The best solution I could think of for this is to look for AT_BASE in the auxiliary vector, which should point at the dynamic linker, but I haven't gotten around to that yet, as it requires setting up some plumbing. If you don't feel like doing that yourself, I suggest we just move that check into a separate test, and mark it as @skipIfLinux.

The other part that's not clear to me is why you needed to do the function pointer trick in the lldb-mi test. I understand it was needed for lldb-server because it does not know anything about debug info, but it's not clear to me why would this apply to lldb-mi...

packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
81–83

The comment should be updated as well. Or, you could make this self documenting and write something like:

has_stub_name = not self.isMIPS() and not self.isPPC64le()
if has_stub_name: ...
lbianc added a comment.Mar 6 2018, 6:11 AM

About the environ symbol issue, I'm going to move to a separate test method in the same test file, is it ok?

The function pointer trick is necessary for power architecture, because it has a global entry point, which is not executed when calling the function locally (i. e. in the sabe context). There is a discussion about it here https://reviews.llvm.org/D43768?id=135917.
In this test case, the option skip-prologue is turned off before creating the breakpoint to that function, so consider the function:

<ns::foo1()>    lis     r2,4098
<ns::foo1()+4>  addi    r2,r2,32512
<ns::foo1()+8>  mflr    r0
<ns::foo1()+12> std     r31,-8(r1)
<ns::foo1()+16> std     r0,16(r1)
<ns::foo1()+20> stdu    r1,-112(r1)
<ns::foo1()+24> mr      r31,r1
<ns::foo1()+28> addis   r3,r2,-2
<ns::foo1()+32> addi    r3,r3,-30076

The global entry point is:

<ns::foo1()>    lis     r2,4098
<ns::foo1()+4>  addi    r2,r2,32512

And the prologue is (note that the global entry point is included):

<ns::foo1()>    lis     r2,4098
<ns::foo1()+4>  addi    r2,r2,32512
<ns::foo1()+8>  mflr    r0
<ns::foo1()+12> std     r31,-8(r1)
<ns::foo1()+16> std     r0,16(r1)
<ns::foo1()+20> stdu    r1,-112(r1)
<ns::foo1()+24> mr      r31,r1

If the skip-prologue is turned off, the breakpoint will be placed in the first instruction:

<ns::foo1()>    lis     r2,4098

which is the global entry point that will not be executed when calling this function locally, see the call instruction:

<main(int, char const**)+128>        bl      0x100006f8 <ns::foo2()+8>

so the breakpoint is never reached.
Using the function pointer necessarily will execute the global entry point, so that is why I needed that.

labath added a comment.Mar 6 2018, 6:31 AM

About the environ symbol issue, I'm going to move to a separate test method in the same test file, is it ok?

Yes, that's fine.

The function pointer trick is necessary for power architecture, because it has a global entry point, which is not executed when calling the function locally (i. e. in the sabe context). There is a discussion about it here https://reviews.llvm.org/D43768?id=135917.
In this test case, the option skip-prologue is turned off before creating the breakpoint to that function, so consider the function:

Aha, I see what you mean now. However, I'm not convinced that working around the failure is the right thing to do here. For the lldb-server test that was fine because, all it cares about is setting a breakpoint somewhere in memory and making sure it gets hit -- it doesn't matter if that breakpoint is in a function with a global entry point, or even if it is in a function altogether. And the function pointer trick was just a way to steer the program execution to a well-defined address without resorting to assembly.

However, this looks like a different story. I don't think that whoever created the skip-prologue option even knew local entry points exist, but I think he would be surprised by the behavior you have here. And I am certain that I, as a user, would be very surprised (and upset) if setting skip-prologue=false would cause my breakpoints to not get hit in some (not so infrequent) conditions. So in light of the different entry points on ppc, I think we should read the "skip-prologue" option as "stop as early as possible" (while still making sure that you stop!), and treat this as a bug and not something that should be hacked around.

clayborg added inline comments.Mar 6 2018, 7:34 AM
packages/Python/lldbsuite/test/lldbtest.py
1251

s/is/if/

However, this looks like a different story. I don't think that whoever created the skip-prologue option even knew local entry points exist, but I think he would be surprised by the behavior you have here. And I am certain that I, as a user, would be very surprised (and upset) if setting skip-prologue=false would cause my breakpoints to not get hit in some (not so infrequent) conditions. So in light of the different entry points on ppc, I think we should read the "skip-prologue" option as "stop as early as possible" (while still making sure that you stop!), and treat this as a bug and not something that should be hacked around.

Ok, that was one solution we have imagined. I'm going to revert the change of the function pointer for this patch, and create another one with the solution you asked, as it would be in a different context than testing.
Thanks!

If you need to force something to use a global entry point you might be able to switch this test to use a shared library that contains that function. This would then require that the global entry point is used?

If you need to force something to use a global entry point you might be able to switch this test to use a shared library that contains that function. This would then require that the global entry point is used?

Probably yes. We can think about a test case after the change to do not apply the breakpoint in the global entry point using shared library.

lbianc updated this revision to Diff 137236.Mar 6 2018, 10:53 AM

Reverted changes of
packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/main.cpp.

Added a new test function for testing the value of environ symbol.

Other minor changes.

lbianc marked 2 inline comments as done.Mar 6 2018, 10:54 AM
labath accepted this revision.Mar 7 2018, 1:19 AM
This revision is now accepted and ready to land.Mar 7 2018, 1:19 AM
lbianc edited the summary of this revision. (Show Details)Mar 16 2018, 12:01 PM

@labath Could you please commit this change?

This revision was automatically updated to reflect the committed changes.