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

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: JDK-8163808 fix vtable assertion and logging for older classfiles
From:       Karen Kinnear <karen.kinnear () oracle ! com>
Date:       2016-08-19 16:07:32
Message-ID: 53412AFA-61E1-4A25-8B5A-1C9E6765E9E9 () oracle ! com
[Download RAW message or body]

Good timing. Fixed the copyright.

And you are right that I did not need the compiler directive - with jigsaw we don't \
need the -Dignore.symbol.file anymore because we handle this with module boundaries. \
jtreg added an -XaddExports:java.bae/jdk.internal.org.objectweb.asm=ALL_UNNAMED for \
me.

thanks,
Karen

> On Aug 16, 2016, at 10:41 AM, Coleen Phillimore <coleen.phillimore@oracle.com> \
> wrote: 
> 
> http://cr.openjdk.java.net/~acorn/8163808.hs.1/webrev/test/runtime/TransitiveOverrideCFV50/TransitiveOverrideCFV50.java.html \
> <http://cr.openjdk.java.net/~acorn/8163808.hs.1/webrev/test/runtime/TransitiveOverrideCFV50/TransitiveOverrideCFV50.java.html>
>  
> One tiny thing in case you haven't pushed it yet.   The copyright dates should just \
> say 2016, since it's a new test. 
> Also, I'm not sure if you need @compile directive since I think jtreg adds \
> -Dignore.symbol.file for you. 
> Thanks,
> Coleen
> 
> On 8/12/16 3:28 PM, Karen Kinnear wrote:
> > Added a targeted test case for class files with different class file versions in \
> > the inheritance hierarchy.
> > 
> > http://cr.openjdk.java.net/~acorn/8163808.hs.1/webrev/ \
> > <http://cr.openjdk.java.net/%7Eacorn/8163808.hs.1/webrev/> 
> > thanks,
> > Karen
> > 
> > > On Aug 12, 2016, at 1:46 PM, Coleen Phillimore <coleen.phillimore@oracle.com \
> > > <mailto:coleen.phillimore@oracle.com>> wrote: 
> > > 
> > > 
> > > On 8/12/16 1:33 PM, Karen Kinnear wrote:
> > > > Coleen,
> > > > 
> > > > Good catch - I will make that change.
> > > > Today this code is not called for arrays, but I totally appreciate you \
> > > > looking at the bigger picture and preparing for potential other uses.
> > > > 
> > > > 
> > > > Here is the updated lines:
> > > > KlassHandle vtklass_h = vt->klass();
> > > > Klass* vtklass = vtklass_h();
> > > > if (vtklass->is_instance_klass() &&
> > > > (InstanceKlass::cast(vtklass)->major_version() >= \
> > > > klassVtable::VTABLE_TRANSITIVE_OVERRIDE_VERSION)) { assert(method() != NULL, \
> > > > "must have set method"); }
> > > > 
> > > This looks good.
> > > Thanks,
> > > Coleen
> > > 
> > > > Thanks!
> > > > Karen
> > > > 
> > > > > On Aug 12, 2016, at 10:47 AM, Coleen Phillimore \
> > > > > <coleen.phillimore@oracle.com <mailto:coleen.phillimore@oracle.com>> wrote: \
> > > > >  
> > > > > http://cr.openjdk.java.net/~acorn/8163808.hs/webrev/src/share/vm/oops/klassVtable.cpp.udiff.html \
> > > > > <http://cr.openjdk.java.net/%7Eacorn/8163808.hs/webrev/src/share/vm/oops/klassVtable.cpp.udiff.html>
> > > > >  
> > > > > void vtableEntry::verify(klassVtable* vt, outputStream* st) {
> > > > > NOT_PRODUCT(FlagSetting fs(IgnoreLockingAssertions, true));
> > > > > + KlassHandle vtklass_h = vt->klass();
> > > > > + Klass* vtklass = vtklass_h();
> > > > > + if (InstanceKlass::cast(vtklass)->major_version() >= \
> > > > > klassVtable::VTABLE_TRANSITIVE_OVERRIDE_VERSION) { assert(method() != NULL, \
> > > > > "must have set method"); + }
> > > > > 
> > > > > I might be wrong but the vtable->klass() can be an ArrayKlass, so I think \
> > > > > you have to do: 
> > > > > if (vtklass->oop_is_instance() && InstanceKlass::cast(vtklass) ...)
> > > > > 
> > > > > InstanceKlass::cast makes this assertion.  Otherwise, the code looks good.
> > > > > 
> > > > > Coleen
> > > > > 
> > > > > On 8/11/16 5:07 PM, Karen Kinnear wrote:
> > > > > > Please review:
> > > > > > https://bugs.openjdk.java.net/browse/JDK-8163808 \
> > > > > > <https://bugs.openjdk.java.net/browse/JDK-8163808> 
> > > > > > http://cr.openjdk.java.net/~acorn/8163808.hs/webrev \
> > > > > > <http://cr.openjdk.java.net/%7Eacorn/8163808.hs/webrev> 
> > > > > > Bug: For classfiles before class file version 51, JVMS did not support \
> > > > > > transitive over-ride behavior. Implementation needed to check this in \
> > > > > > three places, not just one. Vtable size calculation is only exact for \
> > > > > > later classfile versions. 
> > > > > > Also fixed vtable logging output - since the method name-and-sig printing \
> > > > > > was changed to also print the holder's class name, we do not need to \
> > > > > > print the holder's class name separately - it was printing twice. 
> > > > > > Testing: linux-x64-slowdebug
> > > > > > rbt hs-nightly-runtime.js
> > > > > > jck vm,lang, api.java.lang
> > > > > > small invocation tests
> > > > > > 
> > > > > > thanks,
> > > > > > Karen
> > > > > 
> > > > 
> > > 
> > 
> 


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