Re: RFR (XS): 8213525: new unit test GetLocalVariable/LocalVars is not stable

2018-11-13 Thread Chris Plummer 

  
  
Hi Serguei,
  
  Changes look good.
  
  Chris
  
  On 11/12/18 8:06 PM, serguei.spit...@oracle.com wrote:


  
  On 11/12/18 20:05, serguei.spit...@oracle.com wrote:
  
  
Hi Jc,
  
  
  Thank you a lot for reviewing!
  
  On 11/12/18 09:35, JC Beyler wrote:


  Hi Serguei,


The fix looks good (though I never like commented out
  code, why do we not just remove the lines and add a simple
  comment: "Due to JDK-8213525, we do not test X,Y, and Z
  because of stability isssues").
  


I also normally do not like commented out code.
In this particular case, I considered commented out lines as
part of comment.
They explain what is removed better than any words. :)

Okay, I've removed these lines with the comment.
  
  
  Forgot to tell that I've updated the same webrev:
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/
  
  
  Thanks,
  Serguei
  
  

  
But the underlying question I have that is not really
  explained is : "why is it failing?"; is the spec not
  specific in these cases? is it a bug in the
  compiler/runtime that is not yet fixed to conform to the
  spec? I ask because I would imagine that it might be
  something we would like to fix, no?
  


No.
There is no information from the JIT compiler to return errors
when the LVT is absent.
Moreover, different compilers, modes or tiers differently
represent local values that are out of scope. 

Thanks,
Serguei


  


Thanks,
Jc




  
  
  
On Mon, Nov 12, 2018 at 2:25 AM serguei.spit...@oracle.com
  
  wrote:


   Please, review a fix for:
  https://bugs.openjdk.java.net/browse/JDK-8213525

Webrev:
  http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/


Summary:
  A couple of the checks in new unit test developed for
JDK-8080406
  is not stable.
    It is expected that the type of the local intLoc
  returned by the StackValueCollection
  has
    to be T_CONFLICT as it is out of scope at the point
  where the testLocals() is called:

 int staticMeth(byte byteArg, Object objArg, double dblArg, int intArg) {
 testLocals(Thread.currentThread());
 {
 int intLoc = ;
 intArg = intLoc;
 }
 return intArg;
 }

  But sometimes the type T_INT is returned instead of T_CONFLICT.
  The fix is to disable the checks that can fail because of it.


Thanks,
Serguei


  

  
  
  
  
  -- 
  

  
  
  Thanks,
  Jc

Re: RFR (XS): 8213525: new unit test GetLocalVariable/LocalVars is not stable

2018-11-13 Thread serguei.spit...@oracle.com 

  
  
Thanks a lot, Jc!
  Serguei
  
  On 11/13/18 08:59, JC Beyler wrote:


  Ok makes sense then: it is not specified what
happens to locals that are out of scope and therefore depending
on the compiler/modes/tiers, you could get a different return in
those cases.


Webrev looks good to me now :)
Jc
  
  
  
On Mon, Nov 12, 2018 at 8:06 PM serguei.spit...@oracle.com 
  wrote:


  
On
  11/12/18 20:05, serguei.spit...@oracle.com
  wrote:


  Hi Jc,


Thank you a lot for reviewing!

On 11/12/18 09:35, JC Beyler wrote:
  
  
Hi Serguei,
  
  
  The fix looks good (though I never like commented
out code, why do we not just remove the lines and
add a simple comment: "Due to JDK-8213525, we do not
test X,Y, and Z because of stability isssues").

  
  
  I also normally do not like commented out code.
  In this particular case, I considered commented out lines
  as part of comment.
  They explain what is removed better than any words. :)
  
  Okay, I've removed these lines with the comment.


Forgot to tell that I've updated the same webrev:
  http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/


Thanks,
Serguei


  

  But the underlying question I have that is not
really explained is : "why is it failing?"; is the
spec not specific in these cases? is it a bug in the
compiler/runtime that is not yet fixed to conform to
the spec? I ask because I would imagine that it
might be something we would like to fix, no?

  
  
  No.
  There is no information from the JIT compiler to return
  errors when the LVT is absent.
  Moreover, different compilers, modes or tiers differently
  represent local values that are out of scope. 
  
  Thanks,
  Serguei
  
  

  
  
  Thanks,
  Jc
  
  
  
  



  On Mon, Nov 12, 2018 at 2:25 AM serguei.spit...@oracle.com

wrote:
  
  
 Please, review a fix for:
    https://bugs.openjdk.java.net/browse/JDK-8213525
  
  Webrev:
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/
  
  
  Summary:
    A couple of the checks in new unit test
  developed for JDK-8080406
is not stable.
  It is expected that the type of the local
intLoc returned by the StackValueCollection
has
  to be T_CONFLICT as it is out of scope at the
point where the testLocals() is called:
  
   int staticMeth(byte byteArg, Object objArg, double dblArg, int intArg) {
 testLocals(Thread.currentThread());
 {
 int intLoc = ;
 intArg = intLoc;
 }
 return intArg;
 }

  But sometimes the type T_INT is returned instead of T_CONFLICT.
  The fix is to disable the checks that can fail because of it.


Thanks,
Serguei

  

  




-- 

  


Thanks,
Jc
  

  
  


  

  
  
  
  
  -- 
  

  
  
  Thanks,
  Jc

Re: RFR (XS): 8213525: new unit test GetLocalVariable/LocalVars is not stable

2018-11-13 Thread JC Beyler 
Ok makes sense then: it is not specified what happens to locals that are
out of scope and therefore depending on the compiler/modes/tiers, you could
get a different return in those cases.

Webrev looks good to me now :)
Jc

On Mon, Nov 12, 2018 at 8:06 PM serguei.spit...@oracle.com <
serguei.spit...@oracle.com> wrote:

> On 11/12/18 20:05, serguei.spit...@oracle.com wrote:
>
> Hi Jc,
>
>
> Thank you a lot for reviewing!
>
> On 11/12/18 09:35, JC Beyler wrote:
>
> Hi Serguei,
>
> The fix looks good (though I never like commented out code, why do we not
> just remove the lines and add a simple comment: "Due to JDK-8213525, we do
> not test X,Y, and Z because of stability isssues").
>
>
> I also normally do not like commented out code.
> In this particular case, I considered commented out lines as part of
> comment.
> They explain what is removed better than any words. :)
>
> Okay, I've removed these lines with the comment.
>
>
> Forgot to tell that I've updated the same webrev:
>
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/
>
>
> Thanks,
> Serguei
>
> But the underlying question I have that is not really explained is : "why
> is it failing?"; is the spec not specific in these cases? is it a bug in
> the compiler/runtime that is not yet fixed to conform to the spec? I ask
> because I would imagine that it might be something we would like to fix, no?
>
>
> No.
> There is no information from the JIT compiler to return errors when the
> LVT is absent.
> Moreover, different compilers, modes or tiers differently represent local
> values that are out of scope.
>
> Thanks,
> Serguei
>
>
> Thanks,
> Jc
>
>
>
> On Mon, Nov 12, 2018 at 2:25 AM serguei.spit...@oracle.com <
> serguei.spit...@oracle.com> wrote:
>
>> Please, review a fix for:
>>   https://bugs.openjdk.java.net/browse/JDK-8213525
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/
>>
>>
>> Summary:
>>   A couple of the checks in new unit test developed for JDK-8080406
>>  is not stable.
>>   It is expected that the type of the local intLoc returned by the 
>> StackValueCollection
>> has
>>   to be T_CONFLICT as it is out of scope at the point where the
>> testLocals() is called:
>>
>>  int staticMeth(byte byteArg, Object objArg, double dblArg, int intArg) 
>> { testLocals(Thread.currentThread()); { int 
>> intLoc = ; intArg = intLoc; } return intArg; 
>> }
>>
>>   But sometimes the type T_INT is returned instead of T_CONFLICT.
>>   The fix is to disable the checks that can fail because of it.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>
> --
>
> Thanks,
> Jc
>
>
>
>

-- 

Thanks,
Jc

Re: RFR (XS): 8213525: new unit test GetLocalVariable/LocalVars is not stable

2018-11-12 Thread serguei.spit...@oracle.com 

  
  
On 11/12/18 20:05,
  serguei.spit...@oracle.com wrote:


  Hi Jc,


Thank you a lot for reviewing!

On 11/12/18 09:35, JC Beyler wrote:
  
  
Hi Serguei,
  
  
  The fix looks good (though I never like commented out
code, why do we not just remove the lines and add a simple
comment: "Due to JDK-8213525, we do not test X,Y, and Z
because of stability isssues").

  
  
  I also normally do not like commented out code.
  In this particular case, I considered commented out lines as part
  of comment.
  They explain what is removed better than any words. :)
  
  Okay, I've removed these lines with the comment.


Forgot to tell that I've updated the same webrev:
  http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/


Thanks,
Serguei


  

  But the underlying question I have that is not really
explained is : "why is it failing?"; is the spec not
specific in these cases? is it a bug in the compiler/runtime
that is not yet fixed to conform to the spec? I ask because
I would imagine that it might be something we would like to
fix, no?

  
  
  No.
  There is no information from the JIT compiler to return errors
  when the LVT is absent.
  Moreover, different compilers, modes or tiers differently
  represent local values that are out of scope. 
  
  Thanks,
  Serguei
  
  

  
  
  Thanks,
  Jc
  
  
  
  



  On Mon, Nov 12, 2018 at 2:25 AM serguei.spit...@oracle.com 
wrote:
  
  
 Please, review a fix for:
    https://bugs.openjdk.java.net/browse/JDK-8213525
  
  Webrev:
    http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/
  
  
  Summary:
    A couple of the checks in new unit test developed for JDK-8080406
is not stable.
  It is expected that the type of the local intLoc
returned by the StackValueCollection
has
  to be T_CONFLICT as it is out of scope at the point
where the testLocals() is called:
  
   int staticMeth(byte byteArg, Object objArg, double dblArg, int intArg) {
 testLocals(Thread.currentThread());
 {
 int intLoc = ;
 intArg = intLoc;
 }
 return intArg;
 }

  But sometimes the type T_INT is returned instead of T_CONFLICT.
  The fix is to disable the checks that can fail because of it.


Thanks,
Serguei

  

  




-- 

  


Thanks,
Jc

Re: RFR (XS): 8213525: new unit test GetLocalVariable/LocalVars is not stable

2018-11-12 Thread serguei.spit...@oracle.com 

  
  
Hi Jc,
  
  
  Thank you a lot for reviewing!
  
  On 11/12/18 09:35, JC Beyler wrote:


  Hi Serguei,


The fix looks good (though I never like commented out code,
  why do we not just remove the lines and add a simple comment:
  "Due to JDK-8213525, we do not test X,Y, and Z because of
  stability isssues").
  


I also normally do not like commented out code.
In this particular case, I considered commented out lines as part of
comment.
They explain what is removed better than any words. :)

Okay, I've removed these lines with the comment.



  
But the underlying question I have that is not really
  explained is : "why is it failing?"; is the spec not specific
  in these cases? is it a bug in the compiler/runtime that is
  not yet fixed to conform to the spec? I ask because I would
  imagine that it might be something we would like to fix, no?
  


No.
There is no information from the JIT compiler to return errors when
the LVT is absent.
Moreover, different compilers, modes or tiers differently represent
local values that are out of scope. 

Thanks,
Serguei


  


Thanks,
Jc




  
  
  
On Mon, Nov 12, 2018 at 2:25 AM serguei.spit...@oracle.com 
  wrote:


   Please, review a fix for:
  https://bugs.openjdk.java.net/browse/JDK-8213525

Webrev:
 
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8213525-unstable-test.1/


Summary:
  A couple of the checks in new unit test developed for JDK-8080406
  is not stable.
    It is expected that the type of the local intLoc
  returned by the StackValueCollection
  has
    to be T_CONFLICT as it is out of scope at the point
  where the testLocals() is called:

 int staticMeth(byte byteArg, Object objArg, double dblArg, int intArg) {
 testLocals(Thread.currentThread());
 {
 int intLoc = ;
 intArg = intLoc;
 }
 return intArg;
 }

  But sometimes the type T_INT is returned instead of T_CONFLICT.
  The fix is to disable the checks that can fail because of it.


Thanks,
Serguei


  

  
  
  
  
  -- 
  

  
  
  Thanks,
  Jc