RFR (S): 8019192: StringIndexOutOfBoundsException: in Class.getSimpleName()

[Vitaly Davidovich]

Chris,

Since you touched getName(), maybe get rid of that superfluous null check
after expandFromVM? The code can just fall through.

Vitaly

Sent from my phone
On Sep 26, 2013 5:47 PM, "Christian Thalinger" <
christian.thalinger at oracle.com> wrote:

>
> On Sep 26, 2013, at 2:28 PM, John Rose <john.r.rose at oracle.com> wrote:
>
> > On Sep 26, 2013, at 1:13 PM, Christian Thalinger <
> christian.thalinger at oracle.com> wrote:
> >
> >> On Sep 26, 2013, at 11:50 AM, Christian Thalinger <
> christian.thalinger at oracle.com> wrote:
> >>
> >>>
> >>> On Sep 26, 2013, at 1:22 AM, Peter Levart <peter.levart at gmail.com>
> wrote:
> >>>
> >>>> On 09/26/2013 01:27 AM, Christian Thalinger wrote:
> >>>>> http://cr.openjdk.java.net/~twisti/8019192/webrev/
> >>>>>
> >>>>> 8019192: StringIndexOutOfBoundsException: in Class.getSimpleName()
> >>>>> Reviewed-by:
> >>>>>
> >>>>> This is a race in MemberName's name and type getters.
> >>>>>
> >>>>> MemberName's type field is of type Object so it can hold different
> objects when it gets filled in from the VM.  These types include String and
> Object[].  On the first invocation the current type if it's not MethodType
> gets converted to a MethodType.
> >>>>>
> >>>>> There is a tiny window where some instanceof check have already been
> done on one thread and then another thread stores a MethodType.  The
> following checkcast then fails.
> >>>>>
> >>>>> The fix is to make name and type volatile and do the conversion in a
> synchronized block.  This is okay because it's only done once.
> >>>>>
> >>>>> src/share/classes/java/lang/invoke/MemberName.java
> >>>>>
> >>>>
> >>>> Hi Christian,
> >>>>
> >>>> Wouldn't it be cleaner that instead of just casting and catching
> ClassCastException, the volatile field 'type' was 1st copied to a local
> variable and then an instanceof check + casting and returning performed on
> the local variable. This would avoid throwing ClassCastException even if it
> is performed only once per MemberName…
> >>>
> >>> Not sure it would be cleaner; depends on the definition of "cleaner".
>  I had similar code as you describe before but I changed it to catch the
> exception.  If people have a strong opinion here I can change it back.
> >>
> >> Here are the two different versions:
> >>
> >> http://cr.openjdk.java.net/~twisti/8019192/webrev.00/
> >> http://cr.openjdk.java.net/~twisti/8019192/webrev.01/
> >
> > I think the second version is better.  The first uses exception
> processing for normal (though rare) control flow, which is questionable
> style.
> >
> > As we discussed offline, the fields do *not* need to be volatile, since
> the code in the synch. block will see an updated state without volatiles,
> and races outside the synch block are benign, leading to the same end-state.
> >
> > Hoisting the field into a local is the best practice here.(***)  That's
> the hand we are dealt with the Java memory model.
> >
> > Reviewed, with those adjustments.
>
> …and there is the according webrev:
>
> http://cr.openjdk.java.net/~twisti/8019192/webrev.02/
>
> >
> > — John
> >
> > (***) P.S. This tickles one of my pet peeves.  A hoisted field value
> should have the same name (either approximately or exactly) as the field,
> as long as the difference is merely the sampling of a value which is not
> expected to change.  (As opposed to a clever saving of a field that is
> destined to be updated, etc.)
> >
> > IDEs and purists may disagree with this, but it always sets my teeth on
> edge to see name changes that obscure consistency of values for the sake of
> some less interesting consistency (scope rules or rare state changes).
>
> The problem I have with this is that some people might think the local
> variable is the instance field.  IDEs help here because they usually
> highlight local variables and instance fields in different colors.  How
> about this:
>
> http://cr.openjdk.java.net/~twisti/8019192/webrev.03/
>
> >
> > Is there at least some standard naming pattern for these things?
>  "typeSnapshot" doesn't do it for me.  "typeVal" is quieter; I prefer
> quieter.  Elsewhere in the same code I have defiantly used plain "type"
> with a shout-out to hostile IDEs, as with MethodHandle.bindTo or
> MethodHandle.asCollector.  Looking for a compromise...
> >
> > (I also firmly believe that constructor parameter names are best named
> with the corresponding field names, even though quibblers of various
> species, IDE/human, note that there are distinctions to be made between
> them.  It is for the sake of this opinion of mine that blank finals can be
> initialized with the form 'this.foo = foo', which some dislike.)
>
> I concur.  This is what I'm doing.
>
> >
> > But it's fine; push it!
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20130926/af2a6557/attachment-0001.html