RFR(S) 8204209: [Graal] Compilation fails during nmethod printing with "assert(bci == 0 || 0 <= bci && bci < code_size()) failed: illegal bci"

[Igor Veresov]

> On Jun 22, 2018, at 8:29 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> On 6/22/18 8:04 AM, Igor Veresov wrote:
>>> On Jun 21, 2018, at 10:02 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>> 
>>> It is a little confusing that BeforeBci is the same as SynchronizationEntryBCI.
>> Yes, they mean the same thing. BeforeBci == SyncronizationEntryBCI == InvocationEntryBci == -1.
>> However BytecodeFrame::BEFORE_BCI() is currently -2. In Infopoints that come from Graal it’s -2 and it has to be changed to -1 when we synthesize ScopeDescs. Unfortunate choice of values that I hope we can soon reconcile but for now we have to do the remapping.
> 
> Okay.
> 
>>> In nmethod.cpp you check SynchronizationEntryBCI while in jvmciCodeInstaller.cpp you check BEFORE_BCI. So one is for legacy Hotspot code and BeforeBci is for Graal code. Right?
>> Right.
>>> 
>>> Spacing of '\' is wrong in jvmciJavaClasses.hpp.
>> Sorry I don’t see it, which line is that?
> 
> They are at the end of new lines and not aligned to other '\’.

Oh, yeah, I see them now. I’ll fix that. Thanks for the review!

igor

> 
>>> 
>>> How you chose the order of fields in jvmciJavaClasses.hpp? Can it be the same as order of values in MethodCompilation (compilerDefinitions.hpp)?
>>> 
>> Currently they are in the order they are defined in BytecodeFrame.java, which they refer to. But the order doesn’t matter, do you want me to change it?
> 
> I did not look on BytecodeFrame.java that is why I had question. Keep in as it is.
> 
> Thanks,
> Vladimir
> 
>> igor
>>> Thanks,
>>> Vladimir
>>> 
>>> On 6/21/18 9:33 PM, Igor Veresov wrote:
>>>> After discussing this with Tom, we decided that it would be a bad idea to change values of final static fields as this would break compatibility (since javac inlines these values). So, for now, we’d have to do re-mapping.
>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8204209/webrev.01
>>>> Thanks,
>>>> igor
>>>>> On Jun 18, 2018, at 10:12 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
>>>>> 
>>>>> CCing to runtime group.
>>>>> 
>>>>> Seems fine to me.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 6/18/18 6:28 PM, Igor Veresov wrote:
>>>>>> Make hotspot tolerate negative placeholder BCIs that are produced by Graal.
>>>>>> Should this fix be deemed acceptable I’ll backport it to graal-jvmci-8.
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8204209
>>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8204209/webrev/
>>>>>> Thanks,
>>>>>> igor