Request for reviews (S): 6912517: JIT bug compiles out (and stops running) code that needs to be run. Causes NPE.

[Vladimir Kozlov]

On 1/6/10 8:31 PM, John Rose wrote:
> Yes, that looks like a correct fix for the bug.
>
> One possible problem is if the line marked "skip it" is executed very often, it could lead to a performance degradation.  How did you ensure that the new guard logic won't get false positives?

I do not understand what false positive you are talking about.
If a memory expression base register does not match (or we can't
verify) value register then using this memory expression for
implicit null check of the value will be incorrect.

Which line you are talking about? First "continue;" or second (else branch)?

+         const MachOper* oper = mach->memory_inputs(base, index);
+         if (oper == NULL || oper == (MachOper*)-1)
+           continue;             // Not an memory op; skip it
+         if (val == base ||
+             val == index && val->bottom_type()->isa_narrowoop())
+           break;                // Found it
+         else
+           continue;             // Skip it

Vladimir

>
> -- John
>
> On Jan 6, 2010, at 7:43 PM, Vladimir Kozlov wrote:
>
>>
>> http://cr.openjdk.java.net/~kvn/6912517/webrev.00
>>
>> Fixed 6912517: JIT bug compiles out (and stops running) code that needs to be run. Causes NPE.
>>
>> Problem:
>> Block::implicit_null_check() does not check that the value
>> is used in memory expression in instructions with embedded
>> memory load. In the bug case the instruction was:
>>
>> CmpP val,(tls+off)
>>
>> Solution:
>> Add the missing check.
>> Add regression test.
>>
>> Reviewed by:
>>
>> Fix verified (y/n): y, test
>>
>> Other testing:
>> JPRT, CTW
>>
>