Page MenuHomePhabricator

Delete ScriptInterpreterObject
ClosedPublic

Authored by zturner on Mar 6 2015, 2:34 PM.

Details

Reviewers
clayborg
Summary

This is some preliminary work as discussed in the previous patch to get Python to be able to properly be split from the rest of the code.

This patch completely deletes ScriptInterpreterObject, and instead coverts ScriptInterpreter and ScriptInterpreterPython to use objects in StructuredData instead. To support this, a new category of StructuredData object is created, a "Generic" category which holds a void*. In PythonDataObjects, we then create a subclass of StructuredData::Generic called StructuredPythonObject whose job is just to Addref and Decref the void* on construction and destruction.

All methods in ScriptInterpreterPython now return StructuredData objects instead of PythonDictionary, PythonList, etc. To support this, the various PythonDataObject classes contain methods to convert them the corresponding StructuredData types. So the Python methods behave as normal, but instead of returning a PythonObject directly, they just convert first.

The only tricky part of this was with container types like PythonList and PythonDictionary. For this, we had to convert the contained elements *and* the container itself, since a client won't be able to use a StructuredList whose members are PythonList void*s, for example.

The rest of the code in this patch is just fixup stuff to get things compiling with StructuredData objects instead of Python objects.

Tested on Windows and Linux, no regressions. Will test on Mac when I can, but my machine is having technical difficulties. But since I didn't see any issues on Linux, I don't expect anything serious.

One thing I'm not sure of is how to test OperatingSystemPython and DynamicRegisterInfo. I don't really know what these are or how they're used. Are there tests for these things in-tree? Should I expect that if all the linux tests pass, that these are ok?

Diff Detail

Event Timeline

zturner updated this revision to Diff 21397.Mar 6 2015, 2:34 PM
zturner retitled this revision from to Delete ScriptInterpreterObject.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: clayborg.
zturner added a subscriber: Unknown Object (MLST).
clayborg edited edge metadata.Mar 6 2015, 2:44 PM

So does this need to be applied after patches 1 and 2 from before?

Before. This is totally independent of patches 1 and 2. Apply this on a
clean repo and then don't apply 1 and 2. This can go in independently.

emaste added a subscriber: emaste.Mar 6 2015, 3:04 PM

I tried building and running the Run Testsuite target in Xcode but it says
it can't find module lldb and that lldb.py needs to be in the Framework
path etc. I suspect I'm just doing this the wrong way since everything
works on Linux.

zturner updated this revision to Diff 21592.Mar 10 2015, 9:29 AM
zturner edited edge metadata.

Rebase against ToT

Did you have a chance to try this out?

Not yet, I am trying to get to it but I have been really busy with other work related stuff lately. Sorry for the delay.

zturner updated this revision to Diff 21956.Mar 13 2015, 2:32 PM

Rebase against ToT again.

zturner updated this revision to Diff 21970.Mar 13 2015, 4:16 PM

Rebase against ToT again, a bunch of more changes just went in for ScriptInterpreter.

Hi Greg, any chance you could get to this this week? Now that i have gtest
working it's of even more importance, because this patch will allow us to
stop linking Python into the unittest exe, which greatly slows its link
time and has some other unwanted side effects on windows.

I think it should be fairly straightforward. No regressions on linux/win,
and compiles on Mac. Only thing I'm not sure about is if
OperatingSystemPython is exercised by linux test suite, because there were
some changes there

zturner updated this revision to Diff 22027.Mar 16 2015, 9:34 AM

Rebase against ToT (r232380)

zturner updated this revision to Diff 22030.Mar 16 2015, 10:02 AM

The last rebase introduced a compiler error. This update fixes it.

clayborg requested changes to this revision.Mar 16 2015, 11:21 AM
clayborg edited edge metadata.

So the operating system plugin isn't working with this. To test this you can debug any program and stop at main and type:

(lldb) settings set target.process.python-os-plugin-path /Volumes/work/gclayton/Documents/src/lldb/trunk/examples/python/operating_system.py
(lldb) thread list

It should then display extra threads that is got from the operating_system.py:

(lldb) settings set target.process.python-os-plugin-path /Volumes/work/gclayton/Documents/src/lldb/trunk/examples/python/operating_system.py
(lldb) thread list 
Process 55813 stopped
* thread #1: tid = 0x219d30, 0x0000000100000b96 a.out main(argc=1, argv=0x00007fff5fbff8a8, envp=0x00007fff5fbff8b8, aapl=0x00007fff5fbff9e8) + 70, stop reason = breakpoint 1.1, queue = com.apple.main-thread
  thread #2: tid = 0x111111111, 0x0000000000000011, name = one, queue = queue1
  thread #3: tid = 0x222222222, 0x0000000000000075, name = two, queue = queue2
  thread #4: tid = 0x333333333, 0x0000000100000000 a.out _mh_execute_header, name = three, queue = queue3
(lldb) settings clear target.process.python-os-plugin-path
(lldb) thread list
Process 55813 stopped
* thread #1: tid = 0x219d30, 0x0000000100000b96 a.out main(argc=1, argv=0x00007fff5fbff8a8, envp=0x00007fff5fbff8b8, aapl=0x00007fff5fbff9e8) + 70, stop reason = breakpoint 1.1, queue = com.apple.main-thread

I will add a test for this.

This revision now requires changes to proceed.Mar 16 2015, 11:21 AM
zturner updated this revision to Diff 22042.Mar 16 2015, 12:49 PM
zturner edited edge metadata.

Fixes some related to OperatingSystemPython.

I added a test that can be used to test the Python OS plugin:

% svn commit
Adding         python_os_plugin
Adding         python_os_plugin/Makefile
Adding         python_os_plugin/TestPythonOSPlugin.py
Adding         python_os_plugin/main.c
Adding         python_os_plugin/operating_system.py
Transmitting file data ....
Committed revision 232401.
clayborg requested changes to this revision.Mar 16 2015, 1:07 PM
clayborg edited edge metadata.

If you run this test on MacOSX, it crashes:

% cd test
% ./dotest.py -t -v functionalities/plugins/python_os_plugin
This revision now requires changes to proceed.Mar 16 2015, 1:07 PM

The test passes prior to these changes.

Yea I'm looking into this right now. I wasn't sure if it was crashing
because OperatingSystemPython had unrelated portability issues on Windows,
so I was reproing this on Linux right now. In any case, it looks like
there's still a few more issues.

zturner updated this revision to Diff 22062.Mar 16 2015, 4:14 PM
zturner updated this object.
zturner edited edge metadata.

Hi Greg,

I think I've fixed the remainder of the issues. The problem this time was that the RegisterData defined in operating_system.py is a string with embedded nulls, and all of PythonString's getters and setters were not written to correctly handle this. So I changed all of the const char*'s here to be StringRefs (since we specifically need to support strings with embedded nulls). Here's a run on Linux with my patch applied.

zturner@zturner-goobuntu:/usr/local/google_ssd/src/llvm/build/ninja$ bin/lldb
(lldb) file a.out
Current executable set to 'a.out' (x86_64).
(lldb) break set -n main
Breakpoint 1: where = a.out`main, address = 0x00000000004004f0
(lldb) run
Process 1681 launched: '/usr/local/google_ssd/src/llvm/build/ninja/a.out' (x86_64)
Process 1681 stopped
* thread #1: tid = 1681, 0x00000000004004f0 a.out`main, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x00000000004004f0 a.out`main
a.out`main:
->  0x4004f0 <+0>: int3   
    0x4004f1 <+1>: movq   %rsp, %rbp
    0x4004f4 <+4>: movl   $0x0, %eax
    0x4004f9 <+9>: movl   $0x0, -0x4(%rbp)
(lldb) settings set target.process.python-os-plugin-path /usr/local/google_ssd/src/llvm/tools/lldb/examples/python/operating_system.py
(lldb) thread list
Process 1681 stopped
* thread #1: tid = 1681, 0x00000000004004f0 a.out`main, name = 'a.out', stop reason = breakpoint 1.1
  thread #2: tid = 4581298449, 0x0000000000000011, name = 'one', queue = 'queue1'
  thread #3: tid = 9162596898, 0x0000000000000075, name = 'two', queue = 'queue2'
  thread #4: tid = 13743895347, name = 'three', queue = 'queue3'
(lldb)

Note this still doesn't work on Windows because operating_system.py checks the triple and only returns valid information for x86-64, which I haven't added support for yet. Also, I ran the new test on Linux and it passes.

Which strings have embedded NULLs??? I don't see any.

Which strings have embedded NULLs??? I don't see any.

It's the calls to struct.pack() in get_register_data(). For example, this line:

struct.pack('21Q',1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21)

says "there are 21 items of type Q following, where Q means uint64. Format them as such and return the result as a string". So you get a bunch of 0s in the resulting string.

Ahhh, I see. I am running the test suite now with these changes on MacOSX.

By the way, I still have trouble running tests on OSX. I'm not really sure
what the recommended workflow is. Could we add a section to the website
called "Running LLDB Tests"? I created a bug for it with some things that
would be useful to have documented in such a page:

https://llvm.org/bugs/show_bug.cgi?id=22887

We wouldn't need to tackle this all at once, but it would be nice if I
didn't have to keep asking you to run the test suite on OSX for me.

clayborg accepted this revision.Mar 17 2015, 10:05 AM
clayborg edited edge metadata.

Test suite ran cleanly on MacOSX.

This revision is now accepted and ready to land.Mar 17 2015, 10:05 AM

Very nice. Do you plan to review the diff or are you ok with this as is?

Looks good as is.

zturner closed this revision.Oct 15 2015, 1:51 PM