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

[Christian Thalinger]

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/95f0cb85/attachment.html