[Lldb-commits] [PATCH] C++ API tests review request

Malea, Daniel daniel.malea at intel.com
Fri May 3 13:14:08 PDT 2013 

Alright, here's the fixed tests. Turns out that race condition that I made
by registering a listener after the process was created was causing some
issues in one of the tests. It's all fixed up now though; I am providing a
listener on process creation and am seeing more tests pass.

The consistently failing case is test_breakpoint_callback. I can reproduce
this failure on Linux/Mac OS X, so I wonder if there's still something
wrong in the way I use the API here.

On Linux, test_listener_resume fails *sometimes* but not always with an
assertion failure:
source/Plugins/Process/POSIX/ProcessPOSIX.cpp:225: virtual
lldb_private::Error ProcessPOSIX::DoResume(): Assertion `state ==
eStateStopped || state == eStateCrashed' failed.

Which smells like another race condition, possibly in the Linux/POSIX side
of things.


On Mac OS X, I *sometimes* see test_listener_event_description fail due to
not being able to query any function name from within the listener thread.
I will look at this test further, but if someone with fresh eyes wants to
take a look, that might be useful as well.


Thanks,
Dan



On 2013-05-03 1:59 PM, "jingham at apple.com" <jingham at apple.com> wrote:

>I'm a little confused here.  Looks like you are stalling in trying to get
>the "private run lock" that Greg introduced a little while back.  That is
>currently #ifdef'ed __APPLE__, at least in my sources.  That definitely
>has some problems still, and is what I'm working to sort out right now.
>But it shouldn't be active for you if you are on Linux.
>
>Jim
>
>On May 3, 2013, at 10:46 AM, "Malea, Daniel" <daniel.malea at intel.com>
>wrote:
>
>> So I tried your suggestions of using SBTarget.Launch(listener,..) to
>> attach a listener at construction, and also tried using
>> SBDebugger.GetListener() after launching the process with
>>LaunchSimple(),
>> but these approaches don't yield good results. Specifically, I'm now
>> seeing every test (that uses listeners) deadlock:
>> 
>> #0  pthread_rwlock_wrlock () at
>> ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_rwlock_wrlock.S:83
>> #1  0x00007f150e30a5fe in lldb_private::ReadWriteLock::WriteLock
>> (this=0x1958b50) at ../tools/lldb/include/lldb/Host/ReadWriteLock.h:77
>> #2  0x00007f150e304c46 in lldb_private::Process::PrivateResume
>> (this=0x1958200) at ../tools/lldb/source/Target/Process.cpp:3282
>> #3  0x00007f150e2fff7c in lldb_private::Process::Resume (this=0x1958200)
>> at ../tools/lldb/source/Target/Process.cpp:1677
>> #4  0x00007f150e08b732 in lldb::SBTarget::Launch (this=0x7fff2dc86d60,
>> listener=..., argv=0x0, envp=0x0, stdin_path=0x0, stdout_path=0x0,
>> stderr_path=0x0, working_directory=0x195ca30
>> "/home/daniel/dev/llvm/tools/lldb/test/api/multithreaded",
>>launch_flags=0,
>> stop_at_entry=false, error=...)
>>    at ../tools/lldb/source/API/SBTarget.cpp:712
>> #5  0x00007f150e08b171 in lldb::SBTarget::LaunchSimple
>> (this=0x7fff2dc86d60, argv=0x0, envp=0x0, working_directory=0x195ca30
>> "/home/daniel/dev/llvm/tools/lldb/test/api/multithreaded") at
>> ../tools/lldb/source/API/SBTarget.cpp:603
>> #6  0x0000000000404115 in test (dbg=..., args=...) at
>> test_breakpoint_callback.cpp:48
>> #7  0x0000000000402f78 in main (argc=2, argv=0x7fff2dc86f48) at
>> driver.cpp:30
>> 
>> 
>> 
>> 
>> It looks quite similar to the other deadlock we were discussing
>>yesterday
>> in the context of that ThreadPlanBase patch. Anyways, I have not yet
>>been
>> able to enable LLDB logging in these test cases, but I imagine it might
>>be
>> useful to see what is going awry here. I have not yet had a chance to
>>try
>> it on Mac OS X yet, but that is my next step; it should help clarify if
>> it's something related to POSIX/Linux plugins (like the WillResume
>> function Greg mentioned) in generic code.
>> 
>> I'm nonetheless attaching the updated patch that attaches a listener on
>> Process construction in listener_test.cpp:49. Maybe you can see
>>something
>> else wrong with it?
>> 
>> Cheers,
>> Dan
>> 
>> 
>> 
>> On 2013-05-03 12:03 PM, "jingham at apple.com" <jingham at apple.com> wrote:
>> 
>>> 
>>> On May 3, 2013, at 8:51 AM, "Malea, Daniel" <daniel.malea at intel.com>
>>> wrote:
>>> 
>>>> I see; that makes sense. I was copying the approach used by
>>>> examples/python/process_events.py:
>>>> 
>>>> 178                 listener = lldb.SBListener("event_listener")
>>>> 
>>>> 179                 # sign up for process state change events
>>>> 
>>>> 180                 process.GetBroadcaster().AddListener(listener,
>>>> lldb.SBProcess.eBroadcastBitStateChanged)
>>>> 
>>>> 
>>>> 
>>>> Which seems to work, but now I'm wondering if that's correct? Should
>>>>it
>>>> be
>>>> fixed, or is there some magic that allows that particular python code
>>>>to
>>>> create a new listener and register it?
>>> 
>>> I was just about to tell you not to copy that test case.
>>> 
>>> We put in some asserts to make sure we weren't unlocking the unlocked
>>> write end of the run lock (they are in TOT but turned off at present.)
>>> With them turned on this test asserts,.  I'll have to fix it as I go
>>> through and clean up all the places where these asserts fail.
>>> 
>>>> 
>>>> I will try using the debugger's listener instead of my own and update
>>>> the
>>>> patch, as well as the test pass/fail status.
>>>> 
>>> 
>>> Yes, that would be best.  At some point it would be great to get back
>>>to
>>> allowing multiple listeners for the Process events.  But unlike most
>>> other events in lldb, things happen when you fetch the events from the
>>> event queue, so it is confusing when various agents are allowed to pull
>>> then off.  If we want to do this, we should probably make it so that
>>>the
>>> one of the listeners is the driver, and the others wait to get notified
>>> till the event has been delivered to the driving listener or something
>>> like that.  But I don't think this is a terribly important task.
>>> 
>>> Jim
>>> 
>>>> Thanks,
>>>> Dan
>>>> 
>>>> 
>>>> 
>>>> On 2013-05-03 11:36 AM, "jingham at apple.com" <jingham at apple.com> wrote:
>>>> 
>>>>> BTW, just to give a little background on this.  If there are no
>>>>> listeners
>>>>> for an event, Broadcasters in lldb just discard them.  That's useful,
>>>>> for
>>>>> instance if nobody cares about breakpoint changed events, there's no
>>>>> reason for the target to hold onto all of them just in case somebody
>>>>> might later care.  But that means if you start up a process with no
>>>>> listener, then there's a race condition with whether your listener
>>>>>will
>>>>> be in time to get the initial stopped event.  If it gets added after
>>>>> the
>>>>> process is launched and stopped, for instance, it could miss it.
>>>>> 
>>>>> I was going to solve that by having each Broadcaster have a "Prime a
>>>>> new
>>>>> listener with relevant events" method, so that the Process
>>>>>Broadcaster
>>>>> would hold onto all the events it had gotten till some listener
>>>>>signed
>>>>> up
>>>>> with it.  In fact, the method is still there
>>>>> (Broadcaster::AddInitialEventsToListener.)  But Greg didn't like that
>>>>> in
>>>>> the case of the Process for what are I am sure good reasons though I
>>>>> can
>>>>> no longer recall them.  So we decided to just force the Process to be
>>>>> created with a listener.
>>>>> 
>>>>> Jim
>>>>> 
>>>>> On May 3, 2013, at 8:29 AM, jingham at apple.com wrote:
>>>>> 
>>>>>> The way the SB API's work for creating processes, if you don't
>>>>>> register
>>>>>> a listener at construction, your Process gets hooked up to the
>>>>>> Debugger's listener, which you can get with SBDebugger.GetListener.
>>>>>> So
>>>>>> you do have two.  Better to either construct it with a listener
>>>>>> explicitly, or use the debugger's.
>>>>>> 
>>>>>> Jim
>>>>>> 
>>>>>> On May 3, 2013, at 5:50 AM, "Malea, Daniel" <daniel.malea at intel.com>
>>>>>> wrote:
>>>>>> 
>>>>>>> Thanks for the heads up; will keep this in mind. The tests only
>>>>>>> register one SBListener with each process; but it's not done at
>>>>>>> construction, but via:
>>>>>>> 
>>>>>>> Process.GetBroadcaster.AddListener(listener,
>>>>>>> eBroadcastBitStateChanged);
>>>>>>> 
>>>>>>> However, I should note that the tests call the listener's
>>>>>>> waitForEvent() from another thread, which I imagine is a popular
>>>>>>>use
>>>>>>> case for frontends.
>>>>>>> 
>>>>>>> 
>>>>>>> Any comments appreciated; the (svn) patch is attached to the
>>>>>>>previous
>>>>>>> email (addressed to Greg/the list) in this thread.
>>>>>>> 
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> Dan
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> -----Original Message-----
>>>>>>> From: jingham at apple.com [mailto:jingham at apple.com]
>>>>>>> Sent: Thursday, May 02, 2013 6:04 PM
>>>>>>> To: Malea, Daniel
>>>>>>> Cc: lldb-commits at cs.uiuc.edu
>>>>>>> Subject: Re: [Lldb-commits] [PATCH] C++ API tests review request
>>>>>>> 
>>>>>>> One thing to be wary of with the process and listeners is that
>>>>>>> although it is possible to have two listeners listening to process
>>>>>>> events, and at some point that should work, at present it doesn't
>>>>>>> work
>>>>>>> all that well.  For the nonce, if you are going to write test cases
>>>>>>> that involve consuming process events, just use the listener that
>>>>>>>you
>>>>>>> passed to the process when you created it.
>>>>>>> 
>>>>>>> Jim 
>>>>>>> 
>>>>>>> 
>>>>>>> On May 2, 2013, at 2:55 PM, "Malea, Daniel"
>>>>>>><daniel.malea at intel.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>>> Hi all,
>>>>>>>> 
>>>>>>>> I'm trying to nail down exactly what multithreaded problems there
>>>>>>>> are
>>>>>>>> with the LLDB API, so I wrote some tests that use it via C++. The
>>>>>>>> same
>>>>>>>> stuff could have been done from Python, but I figured since
>>>>>>>>there's
>>>>>>>> so
>>>>>>>> few C++ tests (one that I found "check_public_api_headers") I
>>>>>>>> figured
>>>>>>>> I'd write some to keep things interesting.
>>>>>>>> 
>>>>>>>> When someone has a moment, could you provide some feedback, as I'm
>>>>>>>> seeing 3 out of 4 tests fail on Linux/Mac OS X - wondering if
>>>>>>>>these
>>>>>>>> are LLDB bugs I'm uncovering, or more likely, that the tests are
>>>>>>>> using
>>>>>>>> the API incorrectly.
>>>>>>>> 
>>>>>>>> Here's a brief description of what the tests do:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 1.  Test that a function registered with
>>>>>>>>SBBreakpoint.SetCallback()
>>>>>>>> is called when a breakpoint is hit. -- the function is not invoked
>>>>>>>> 2.  Test that an SBListener registered with a process receives an
>>>>>>>> event.
>>>>>>>> 3.  Test that a process retrieved from an SBEvent can be used to
>>>>>>>> query thread/frame information - retrieving thread/frame works ok,
>>>>>>>> but querying a function name does not  4.  Test that a process can
>>>>>>>> be
>>>>>>>> resumed from a thread that did not start it - This one's weird:
>>>>>>>> SBProcess::GetState() returns eStateStopped() but calling
>>>>>>>> Process::Resume() fails with error "process is running".
>>>>>>>> 
>>>>>>>> Currently, only #2 passes, which is the simplest case. There's a
>>>>>>>> bunch of boiler plate code in there that is not too interesting.
>>>>>>>>The
>>>>>>>> meat of the test logic are in the .cpp files that are prefixed
>>>>>>>>with
>>>>>>>> "test_".
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks a bunch,
>>>>>>>> Dan
>>>>>>>> 
>>>>>>>> _______________________________________________
>>>>>>>> lldb-commits mailing list
>>>>>>>> lldb-commits at cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>> 
>> 
>> <test_api_multithreaded_v2.patch>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_api_multithreaded_v3.patch
Type: application/octet-stream
Size: 17335 bytes
Desc: test_api_multithreaded_v3.patch
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130503/de468519/attachment.obj>

More information about the lldb-commits mailing list