[lldb-dev] Locking issues on windows

[Malea, Daniel]

So, it looks like the locks are going awry in several places.

Carlo, I can confirm that your fix resolves some of the hangs that
everyone is experiencing but not all. Specifically, the
TestInlineStepping.py seems to still deadlock on the acquisition of one of
the Process (public) locks during a Resume(). That said, toggling the lock
in the constructor doesn't seem like a sound workaround..


Greg,

179329 is the commit that seems to have made things go all sideways. After
that commit, no Debian users can install a package that doesn't deadlock
on startup, we have no visibility into the testing status on the
buildbots, and the commit itself seems scary as it exposes a reference to
one of two internal locks to users based on what thread they're running in.

After briefly studying the Process class, I'm a little worried about the
complexity of the design. Could you explain the reason 2 different R/W
locks are needed? I understand why one R/W lock makes sense in the class,
but two seem overly complicated.

You mentioned that you'll improve the R/W (scoped?) locking classes.. Any
reason to not use boost (or some other C++11 library) for this? If we do
have to roll our own in LLDB, the lack of tests is worrisome.


If the improvements to the R/W locker classes you've got in progress don't
allow the test suite to run to completion, could you please revert 179329
until we have something that allows us to run the tests? Lots of patches
are backed up atm due to the LLVM practice of not committing on top of a
broken trunk.


Dan

PS. The hanging buildbots to watch are:

http://lab.llvm.org:8011/builders/lldb-x86_64-darwin11/builds/1890
http://lab.llvm.org:8011/builders/lldb-x86_64-debian-clang

http://lab.llvm.org:8011/builders/lldb-x86_64-linux


On 2013-04-17 12:47 PM, "Greg Clayton" <gclayton at apple.com> wrote:

>
>On Apr 17, 2013, at 1:27 AM, Carlo Kok <ck at remobjects.com> wrote:
>
>> I'm trying to update the Windows branch to the latest and greatest and
>>found these locking issues (not sure if they're relevant for posix too):
>> 
>> When I attach a process (I only use the gdb remote) the first even I
>>get is "stopped" which tries to unlock m_private_run_lock, however this
>>one is never locked in the first place. Windows' writelock doesn't
>>appreciate that; as a workaround I added a
>> m_private_run_lock.WriteLock(); in Process' constructor, which seems to
>>fix that.
>
>We need to fix this better by locking the private run lock when attaching
>if all goes well.
>
>> 
>> The second issue occurs when when trying to cause a "Stop" when it's
>>already paused on internal breakpoints; for me this is during slow
>>symbol load. When happens is that the loading (which happens from within
>>Process::ShouldBroadcastEvent) resumes it, then the process exits
>>properly (triggers the ShouldBroadcastEvent again) however:
>> 
>> ProcessEventData::DoOnRemoval(lldb_private::Event * event_ptr)
>> called by Listener::FindNextEventInternal.
>> 
>> The resume call is in this condition:
>>  if (state != eStateRunning)
>
>Where is the above "if (state != eStateRunning)"?
>
>> Changing that to:
>> lldb::StateType state = m_process_sp->GetPrivateState();
>> if (state != eStateRunning && state != eStateCrashed && state !=
>>eStateDetached && state != eStateExited)
>
>There are functions that indicate if the function is stopped or running.
>We should use those functions. (search for "StateIsStopped").
>
>> 
>> Seems to fix it, as there's no reason to try & resume a process that's
>>not running in the first place (and since exiting doesn't unlock a
>>process this causes a deadlock)
>> 
>> The last issue is this:
>> void * Process::RunPrivateStateThread ()
>> does : m_public_run_lock.WriteUnlock(); when it's done. The Finalize
>>also unlocks that same lock, which Windows crashes on.
>> commenting that out and it seems to work stable.
>
>We need to build in some smarts into our Read/Write locking class to know
>if the read/write lock is taken and only unlock if the corresponding
>read/write lock is locked. I will make this change today.
>
>> 
>> 
>> Anyone see any issues in all of this? (might make sense to apply this
>>to trunk too; it's never good to have unbalanced lock/unlocks)
>> _______________________________________________
>> lldb-dev mailing list
>> lldb-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>
>_______________________________________________
>lldb-dev mailing list
>lldb-dev at cs.uiuc.edu
>http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev