[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8087342: crash in klassItable_initialize_itable_for_interface
From:       Karen Kinnear <karen.kinnear () oracle ! com>
Date:       2015-08-04 18:01:23
Message-ID: 22E26B36-E502-4FE1-95C9-C7343BEB04F4 () oracle ! com
[Download RAW message or body]

Lois,

Many thanks for the review and for testing this on windows.

I rewrote the comments on the lines you suggested to be clearer how to create from
javac. Thank you for the suggestion.

I will check this in now.
thanks,
Karen

  static int m() { return 1;}  // javac with "n()" and patch to "m()"
  private int m() { return 2;} // javac with public and patch to private
}

public class D {
  public static int CallStatic() {
    int staticret = C.m();    // javac with "C.n" and patch to "C.m"
...
On Aug 4, 2015, at 1:38 PM, Lois Foltan wrote:

> 
> On 7/31/2015 2:01 PM, Karen Kinnear wrote:
> > Lois,
> > 
> > Here is an updated webrev. The hotspot code has not changed (except for the fixed \
> > comments). I added a test to investigate if I could have static and instance \
> > fields in the same class (obviously with -Xverify:none). I did manage to have 1 \
> > public static, 1 private instance and 1 overpass for an AME from an abstract \
> > interface. Obviously none of this matches the jls, or jvms or passes the verifier \
> > - but I did want to make sure the code did the right thing.
> > 
> > The test passes in product and fastdebug. I would appreciate a review for the \
> > test itself. Thanks to Harold for jasm example :-)
> > 
> > updated webrev: http://cr.openjdk.java.net/~acorn/8087342.3/webrev/ \
> > <http://cr.openjdk.java.net/%7Eacorn/8087342.3/webrev/>
> 
> Hi Karen,
> Test looks good.  I applied your patch to a Windows build to see if I could trigger \
> a failure due to a local method array that had a different layout, but it looks \
> good, the test passed with both fastdebug and product builds.  Minor comments: 
> TestStaticandInstance.java:
> line #99 - I think "n" should be "m"
> line #100 - I think the "public" should be "private"
> line #105 - "C.n()" should be "C.m()"
> 
> At least that is the way it seems to be implemented in ASM.
> 
> Thanks,
> Lois
> 
> > > > bug: https://bugs.openjdk.java.net/browse/JDK-8087342
> > 
> > thanks,
> > Karen
> > 
> > On Jul 16, 2015, at 6:03 PM, Karen Kinnear wrote:
> > 
> > > Lois,
> > > 
> > > Thank you for the detailed review. I really appreciate it.
> > > On Jul 15, 2015, at 8:12 PM, Lois Foltan wrote:
> > > 
> > > > 
> > > > On 7/15/2015 12:40 PM, Karen Kinnear wrote:
> > > > > Please review for JDK9:
> > > > > 
> > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8087342
> > > > > webrev: http://cr.openjdk.java.net/~acorn/8087342.2/webrev/ \
> > > > > <http://cr.openjdk.java.net/%7Eacorn/8087342.2/webrev/> 
> > > > > Crash occurs in product when we should through \
> > > > > IncompatibleClassChangeError.
> > > > 
> > > > Hi Karen,
> > > > I think this looks good and I really like how straight forward \
> > > > klassVtable::is_miranda now is.  Some minor clarification comments: 
> > > > - src/share/vm/instanceKlass.cpp
> > > > comments for the find_local_method* were changed to state:
> > > > +// note that the local methods array can have up to one overpass, one static
> > > > +// and one instance (private or not) with the same name/signature
> > > > I think there are two combinations that the code depends on not occurring, \
> > > > they are: 1. all 3 are in existence in the local methods array (one overpass, \
> > > > one static and one instance) 2. the combination of one static and one \
> > > > instance (private or not) In other words there has to be an overpass to cause \
> > > > more than one method with the same name/signature within the local methods \
> > > > array.  And it is either an overpass and a static or an overpass and an \
> > > > instance, but not all 3.  Correct me if I am wrong.
> > > I need to write an additional test to check that. I agree that both the spec \
> > > and the ClassFileParser if _need_verify is set will prevent instance and static \
> > > overlap. I need to see what happens if you skip verification. I will get back \
> > > to you with that and update the comments to clarify if needed.
> > > > 
> > > > - src/share/vm/oops/klassVtable.cpp
> > > Let me see if I can make this clearer - let me know if I can make the comments \
> > > clearer. I truly appreciate your trying to see if this all makes sense and is \
> > > consistent. It is still too complex.
> > > > Thank you for adding the improved comments ahead of is_miranda.  My read is \
> > > > that overpass methods are not considered miranda methods and I agree with \
> > > > that statement.
> > > Yes, they are not considered miranda methods because you don't need to add them \
> > > to the vtable as abstract methods because they already are in the vtable from \
> > > being in the class' LOCAL methods array. So pass 1: overpasses do not exist
> > > pass 2: overpasses are already in the vtable when we calculate mirandas
> > > pass 3: overpasses in a class have the class as their method_holder, not an \
> > > interface, so we aren't looking them up here 
> > > So - pass 2 is the one that cares about the \
> > > find_local_method(Klass:find_overpass vs. Klass::skip_overpass). 
> > > 
> > > > Yet, Klass::find_overpass is specified in the code.  I think the code is \
> > > > correct, but based on the comment I would have thought Klass::skip_overpass \
> > > > should have been specified?
> > > I also think the code is correct.
> > > So what pass 2 is doing is walking through the superinterfaces to see if any of \
> > > the superinterface methods (which all used to be abstract) need to be added to \
> > > the vtable. 
> > > So the question is - what superinterface methods belong in the vtable?
> > > So the searches in is_miranda are designed to find out if there is a method in \
> > > the vtable already such that we don't need to add the superinterface method - \
> > > e.g. this was abstract and we have an implementation for it. 
> > > With the addition of default methods, we have three new challenges - \
> > > overpasses, static interface methods and private interface methods.
> > > 
> > > Static and private interface methods do not get added to the vtable and are not \
> > > seen by the method resolution process. So we skip those.
> > > 
> > > Overpass methods are already in the vtable, so vtable lookup will find them \
> > > there and we don't need to add a miranda method to the end of the vtable. So we \
> > > look for those explicitly. Note that we inherit our superclasses vtable, so the \
> > > superclass' search also needs to use find_overpass.
> > > 
> > > Does this make sense?
> > > 
> > > Is there a way I could make this clearer via comments?
> > > 
> > > > Much like skip_static and skip_private.  So based on your later statement \
> > > > that "Abstract methods may already have been handled via overpasses" it \
> > > > implies that overpass methods, although not miranda methods, can satisfy or \
> > > > stand in for an miranda during pass 2.  So they must be found, did I \
> > > > understand that correctly?
> > > 
> > > > 
> > > > Again, looks good.  I don't need to see another review.  My comments were \
> > > > merely clarification based.
> > > many thanks,
> > > Karen
> > > 
> > > > 
> > > > Thanks,
> > > > Lois
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > testing:
> > > > > internal tests: Defmeth (updated), SelectionResolution - product and \
> > > > > fastdebug jprt
> > > > > jck
> > > > > jtreg/hotspot_all
> > > > > jtreg/jdk_streams
> > > > > test,noncolo.testlist, -atk quick
> > > > > 
> > > > > (jck and macosx testing in progress)
> > > > > 
> > > > > thanks,
> > > > > Karen
> > > > > 
> > > > 
> > > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]