[vmkit-commits] [PATCH/DISCUSS] registerNatives Lock, avoid deadlock

[Nicolas Geoffray]

Oh yes, because getRegisteredNative is called while compiling, and the
class loader lock may be used indirectly by application, we may have a
deadlock situation.

Besides fixing the problem, I think your patch is cleaner because it makes
the locking local. Apologies for suggesting you a bogus implementation. I'm
surprised you encountered the bug that early though! Are you running vmkit
w/openjdk on an application that creates mulitple threads?

Please apply!

Nicolas

On Thu, Nov 3, 2011 at 3:57 PM, Will Dietz <wdietz2 at illinois.edu> wrote:

> Hi,
>
> I'm noticing deadlocks occurring in the OpenJDK port involving the
> registerNatives stuff.
>
> Looking at the lock acquisition/release things look clean (the only
> way to exit without releasing a lock is through an assertion
> failure)--but I don't know what kinds of larger lock-related
> dependencies/assumptions/orderings there might be.  Apparently the
> existing code does do something wrong, however.
>
> Inlined below is a patch that simply gives registerNatives its own
> lock, which seems to resolve these issues in all cases (the deadlock
> occurs reliably in the codes that trigger this).  However I don't like
> "I don't know what was wrong before" bits, and thought I'd bounce this
> off you before deep-diving the locking to see what's wrong.
>
> FWIW the other locks held during deadlock are the protectIR() locks.
>
> Thanks for your time! :)
>
> ~Will
>
> >From d28307a26fdecdddb96273a813451e56216857ab Mon Sep 17 00:00:00 2001
> From: Will Dietz <w at wdtz.org>
> Date: Wed, 2 Nov 2011 17:19:31 -0500
> Subject: [PATCH] Add new 'nativesLock' to protect the registerNatives map.
>
> This fixes various deadlock scenarios when using the other lock.
> Not sure what caused them, but this works for now and is conceptually
> a different lock anyway.
>
> Avoiding debugging the other version for now :).
> ---
>  lib/J3/VMCore/JnjvmClassLoader.cpp |    8 ++++----
>  lib/J3/VMCore/JnjvmClassLoader.h   |    4 ++++
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/lib/J3/VMCore/JnjvmClassLoader.cpp
> b/lib/J3/VMCore/JnjvmClassLoader.cpp
> index 97b598e..59659c5 100644
> --- a/lib/J3/VMCore/JnjvmClassLoader.cpp
> +++ b/lib/J3/VMCore/JnjvmClassLoader.cpp
> @@ -1046,17 +1046,17 @@ JavaString**
> StringList::addString(JnjvmClassLoader* JCL, JavaString* obj) {
>  }
>
>  void JnjvmClassLoader::registerNative(JavaMethod * meth, word_t fnPtr) {
> -  lock.lock();
> +  nativesLock.lock();
>   // Don't support multiple levels of registerNatives
>   assert(registeredNatives.find(meth) == registeredNatives.end());
>
>   registeredNatives[meth] = fnPtr;
> -  lock.unlock();
> +  nativesLock.unlock();
>  }
>
>  word_t JnjvmClassLoader::getRegisteredNative(const JavaMethod * meth) {
> -  lock.lock();
> +  nativesLock.lock();
>   word_t res = registeredNatives[meth];
> -  lock.unlock();
> +  nativesLock.unlock();
>   return res;
>  }
> diff --git a/lib/J3/VMCore/JnjvmClassLoader.h
> b/lib/J3/VMCore/JnjvmClassLoader.h
> index f5891b0..3c1824b 100644
> --- a/lib/J3/VMCore/JnjvmClassLoader.h
> +++ b/lib/J3/VMCore/JnjvmClassLoader.h
> @@ -113,6 +113,10 @@ protected:
>   ///
>   std::map<const JavaMethod*,word_t> registeredNatives;
>
> +  /// nativesLock - Locks the registeredNatives map above
> +  ///
> +  mvm::LockRecursive nativesLock;
> +
>  public:
>
>   /// allocator - Reference to the memory allocator, which will allocate
> UTF8s,
> --
> 1.7.5.1
> _______________________________________________
> vmkit-commits mailing list
> vmkit-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/vmkit-commits/attachments/20111104/c76fbc26/attachment.html>