[llvm-commits] [llvm] r57340 - /llvm/trunk/lib/ExecutionEngine/JIT/JIT.cpp

[Mon Ping Wang]

Hi Nicolas,

Thanks for the explanations on the Java issues.  I'll back out my  
changes and changed my code to avoid this corruption.  It makes sense  
that the JIT lock only for when it changes its internal state.  Anyone  
who uses the default module provider could run into issue when using  
it with the JIT. Do we note this somewhere?  Out of curiosity, does  
the Java compiler grab the JIT lock for its own uses?

   -- Mon Ping



On Oct 10, 2008, at 10:09 AM, Nicolas Geoffray wrote:

> Hi Mon,
>
> The logic here is that materializing a function has to be guarded
> because it creates LLVM IR, constants and types. However, it is not  
> the
> JIT's responsability to guard it, but your module provider  
> implementation.
>
> A default implementation of the module provider only locates the the
> function and creates the IR. So it's ok to let the JIT lock. However
> (and this is what's happening in Java) we can imagine complex things
> where materializing a function requires calling some external code.  
> When
> this complex module provider is able to create the IR after calling  
> this
> external code, *then* it can take the lock.
>
> I acknowledge that it's easier to place the lock in the execution
> engine, but it's wrong :)
>
> Nicolas
>
> Mon Ping Wang wrote:
>> Hi Nicolas,
>>
>> Sorry, I was not aware of Java issues.  I moved the locked upwards
>> because if I didn't, I ran into a problem that materialized function
>> got corrupted occasionally and I got some weird errors.  I'm not sure
>> if this also could occur in Java as it is possible that it will never
>> call getPointerToFunction twice on the same function by different
>> threads.  Is the requirement that materializing the function must be
>> lockless?  If no, maybe we can create a local lock around the
>> materialization so we don't try to do it at the same time.   
>> Otherwise,
>> clients would need guard calls to getPointerToFunction.
>>
>>   -- Mon Ping
>>
>>
>> On Oct 10, 2008, at 1:25 AM, Nicolas Geoffray wrote:
>>
>>
>>> Hi Mon,
>>>
>>> We're having a conflict here.
>>>
>>> Originally, materializing a function was guarded. However, in Java,
>>> materializing a function may execute Java code that use locks, hence
>>> deadlocks can happen.
>>> So I moved the guard after materialization.
>>>
>>> On a more general note I *don't* think materializing a function  
>>> should
>>> be guarded by the JIT compiler. The JIT's mutex should only protect
>>> JIT
>>> internal data. Materializing a function does not modify any JIT  
>>> data.
>>>
>>> Nicolas
>>>
>>>
>>> Mon P Wang wrote:
>>>
>>>> Author: wangmp
>>>> Date: Thu Oct  9 20:47:42 2008
>>>> New Revision: 57340
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=57340&view=rev
>>>> Log:
>>>> Moved guard mutex upwards to guard materializing a function
>>>> in getPointerToFunction
>>>>
>>>>
>>>> Modified:
>>>>   llvm/trunk/lib/ExecutionEngine/JIT/JIT.cpp
>>>>
>>>> Modified: llvm/trunk/lib/ExecutionEngine/JIT/JIT.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/JIT/JIT.cpp?rev=57340&r1=57339&r2=57340&view=diff
>>>>
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> = 
>>>> = 
>>>> ===================================================================
>>>> --- llvm/trunk/lib/ExecutionEngine/JIT/JIT.cpp (original)
>>>> +++ llvm/trunk/lib/ExecutionEngine/JIT/JIT.cpp Thu Oct  9 20:47:42
>>>> 2008
>>>> @@ -489,6 +489,8 @@
>>>>  if (void *Addr = getPointerToGlobalIfAvailable(F))
>>>>    return Addr;   // Check if function already code gen'd
>>>>
>>>> +  MutexGuard locked(lock);
>>>> +
>>>>  // Make sure we read in the function if it exists in this Module.
>>>>  if (F->hasNotBeenReadFromBitcode()) {
>>>>    // Determine the module provider this function is provided by.
>>>> @@ -509,13 +511,11 @@
>>>>      abort();
>>>>    }
>>>>  }
>>>> -
>>>> +
>>>>  if (void *Addr = getPointerToGlobalIfAvailable(F)) {
>>>>    return Addr;
>>>>  }
>>>>
>>>> -  MutexGuard locked(lock);
>>>> -
>>>>  if (F->isDeclaration()) {
>>>>    void *Addr = getPointerToNamedFunction(F->getName());
>>>>    addGlobalMapping(F, Addr);
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits