Page MenuHomePhabricator

Expression evaluation, a new ThreadPlanCallFunctionNoJIT for executing a function call on target via register manipulation
ClosedPublic

Authored by EwanCrawford on Apr 30 2015, 10:35 AM.

Details

Summary

For Hexagon we want to be able to call functions during debugging, however currently lldb only supports this when there is JIT support. Although emulation using IR interpretation is an alternative, it is currently limited in that it can't make function calls.

In this patch we have extended the IR interpreter so that it can execute a function call on the target using register manipulation. Which is the same method GDB uses for calling a function. To do this we need to handle the Call IR instruction, passing arguments to a new thread plan and collecting any return values to pass back into the IR interpreter. The new thread plan is needed to call an alternative ABI interface of "ABI::PerpareTrivialCall()", allowing more detailed information about arguments and return values.

Currently the expression evaluator checks if an expression can be interpreted and if it can it uses the IR interpreter, if it can't then it tries to JIT the expression and run it on target. It seems however that for targets that can JIT, it would be better to use that when executing a function call since its presumably much more robust. Perhaps this should be used exclusively for targets that cannot JIT?

I'd appreciate any feedback since this change touches a few important files.

Diff Detail

Repository
rL LLVM

Event Timeline

EwanCrawford retitled this revision from to Expression evaluation, a new ThreadPlanCallFunctionNoJIT for executing a function call on target via register manipulation.
EwanCrawford updated this object.
EwanCrawford edited the test plan for this revision. (Show Details)
EwanCrawford added reviewers: jingham, spyffe.
EwanCrawford set the repository for this revision to rL LLVM.
EwanCrawford added subscribers: deepak2427, ADodds.
spyffe requested changes to this revision.Apr 30 2015, 10:49 AM
spyffe edited edge metadata.

From the perspective of the IR interpreter, this is a great step forward. After some minor cleanup this should be a good addition.
I would prefer if the API eventually could take arbitrary arguments with LLVM types, but I realize that we're not at that point yet. We can make those improvements in-tree.
I will defer to Jim on the merits of ThreadPlanCallFunctionNoJIT.

source/Expression/IRInterpreter.cpp
1586 ↗(On Diff #24742)

This is not 64-bit clean. Usually lldb::addr_t is the right container for a raw pointer from the inferior.

1591 ↗(On Diff #24742)

At first glance this doesn't look too tricky to solve. Either CallArgument needs to be extended to be able to own data, or (if that is not the right way to do it) you could introduce a piece of storage (a vector of OwningPointers?) with the same lifespan as rawArgs.

1614 ↗(On Diff #24742)

This code is not 64-bit clean.

This revision now requires changes to proceed.Apr 30 2015, 10:49 AM

I'd definitely want a testcase when this gets submitted so that we verify we've caught all the Hexagon-specific aspects of this code.
We'd probably need to add a setting that prevents expressions from using the JIT to enable such a testcase.

jingham edited edge metadata.Apr 30 2015, 12:28 PM

I'd prefer if we could come up with a name that says what this DOES, not what it doesn't do which isn't all that helpful. Maybe ThreadPlanCallFunctionUsingABI? Other than that, this seems fine. Note there are some platforms (the Mach kernel for one) that don't support function calling of any sort. I think this patch is okay in that regard, but you do need to be careful you don't try this method just because JIT'ing isn't possible on such platforms.

Jim

EwanCrawford edited edge metadata.

Thanks for taking the time to review the patch.

In regards to adding a test case I agree it would be valuable but seems awkward to create right now. Since currently just Hexagon implements the necessary ABI functions. So before we could test any other arch we need to implement the alternative ABI::PrepareTrivialCall(), and ABI::GetReturnValueObject() signatures. However AFAIK if interpreting is available then it is chosen over JIT, therefore we wouldn't need a setting to disable JIT.

I could add a test case only working on Hexagon now, to expand on later, but that wouldn't help catch any Hexagon-specific assumptions.

Adding a Hexagon-specific test case sounds like the right thing to do for now. Then whoever enables the necessary ABI support for x86_64 can just enable the testcase.

Sean

ted added a subscriber: ted.May 4 2015, 9:38 AM

The problem with a Hexagon-specific testcase is it would need to use the Hexagon simulator, which is not currently publicly available. We expect it to be available later this year.

Sean, would an XFAIL'd testcase with a binary, to be turned on when the simulator is publicly available, be acceptable?

spyffe added a comment.May 4 2015, 9:50 AM

Ted,

I thought about this a bit. The test case just boils down to testing that you can call a function, right? On x86_64 we should just magically get the IR interpreter handling certain expressions with simple function calls.

So it should be simple enough to make a test case that calls a function with a few integer or pointer arguments. Right now you’d be able to see that testcase work on your simulator, and it would work fine on x86_64 by virtue of running on the JIT. Then, when the necessary ABI support gets into x86_64, we’ll be able to verify it using the same testcase.

Sean

ted edited edge metadata.May 4 2015, 11:13 AM
ted added a subscriber: Unknown Object (MLST).

Thanks for the feedback guys.

I added a test case calling some functions using expression evaluation. As mentioned this will only test the JIT on x86 for now, but would pass using the IR Interpreter on Hexagon.
If you had something else in mind for the test, or any other comments, let me know.

If there are no objections can I go ahead and commit?

EwanCrawford removed rL LLVM as the repository for this revision.

Hi everyone, I'm still quite keen to get this patch through so I'd appreciate any more feedback you have.

Also If I'm adding new files, how do I go about updating the xcode project file?

Do you have a MacOS X system? If so I can explain how to add files in Xcode. If you don't, when you get around to checking this in just ping us and tell us what files you added, and one of us will add them to the project file.

Jim

Hiya Jim, I do have access to an OSX system with Xcode. It would be good to know how to add files for future reference.

Great. I am assuming these files are lldb_private, the instructions are slightly different for SB API files.

Open the lldb.xcworkspace in the top-level lldb directory. Select the Project Navigator from the Navigators on the LHS of the Xcode window, and figure out where in the project hierarchy your new files should go. For instance, if they relate to targets & live processes, they go in Source->Target, if expressions in the Source->Expressions folder, etc. Note, even if the headers & source files are separated into source & include on disk, we put them into the same Group in the project hierarchy.

Anyway, once you've determined what group you want them to go into, select that group in the Project Navigator and do File->Add Files. In the file picker, first select all the .cpp files, and add them. In the dialog that comes up, add the files to the lldb-core Target. Also make sure the files are "relative to Group" but that's the default value so you shouldn't have to change that setting.

Then do it again for the include files, but in that case DON'T add them to any target. The reason for this is that Xcode has some trickery to allow you to access headers as if they were in a directory with an explicit -I, without providing the path relative to include, but then those files wouldn't build with cmake. If you don't add them to the project, then this won't happen...

Note, you can check your work in the Utilities viewer - which you can open by clicking the right-most control on the toolbar - whose tooltip is "Hide or Show Utilities". That will show target ownership, etc.

Then clean & build to make sure everything worked.

Jim

emaste added a subscriber: emaste.Jun 22 2015, 2:22 PM

Is it worth documenting the XCode steps in the www docs? I had the same question some time ago and had someone on the IRC channel walk me through it.

EwanCrawford set the repository for this revision to rL LLVM.

Thanks for the instructions Jim, I agree it would be good to get them documented somewhere.

Adding Greg for review.

Could we please get a final answer on this? Cheers.

I didn't look at the IRInterpreter part of this closely at first, but now looking over this a bit I am concerned about passing the ExecutionContext to CanInterpret, what information is that really providing? Do we expect some threads could interpret but not others? This just makes it confusing. It looks like in fact you only need the architecture, so maybe passing that - or getting it from something you already have your hands on - might be cleaner.

Also a trivial thing, but you do a whole bunch of work in the branch that handles the Call instruction, and then check that the execution context has a process in it, and bail otherwise. It would be cleaner to check that up front.

Finally, I didn't look at this too closely, but for some purposes it is important to ensure that you don't try to run the target at all. We should make sure we weren't accidentally using the fact that Interpret didn't run the target, as part of our strategy for implementing this...

clayborg resigned from this revision.Jul 1 2015, 11:30 AM
clayborg removed a reviewer: clayborg.

I will defer to Jim Ingham on this one. If he is OK with what you have done, I am good.

Thanks for taking the time to have a closer look Jim. Could you expand on your last point about situations where we don't want to run the target at all, it didn't quite click.

To address that final point of not doing any code execution at all for certain targets. I used the CanJIT() idea to create a similar CanInterpretFunctionCalls() member function of Process.
This means that we can just pass the value of the m_can_interpret_function_calls variable to CanInterpret() instead of the Arch or execution context.

Since interpreting function calls is a special case for hexagon right now the value defaults to false, and is only set to true by the hexagon dynamic loader.

In the future though when more targets support this feature, CanInterpretFunctionCalls() would be enabled by default and then specifically disabled by targets.
Using the example of the OSX kernel, which can't do any code execution, the patch explicitly sets jitting and interpreting function calls to false in the dynamic loader using an added CanRunCode() function.

For my part that looks great.

Jim

This revision was automatically updated to reflect the committed changes.