XS RFR: 8009731: loader constraint violation error message

[Karen Kinnear]

Thank you Dan and Coleen.

I added the +1 to buflen for all three messages :-). I did a break in the line after the "="
(for both lines like this one). 

All set now.

many thanks for the multiple reviews,
Karen

On Mar 27, 2013, at 11:10 AM, Daniel D. Daugherty wrote:

> On 3/27/13 8:37 AM, Karen Kinnear wrote:
>> Resending - thanks David for catching that this fix corrected the text and the overwritten string, but
>> did not fix the resolved_klass to be the resolved_method->method_holder()..
>> 
>> http://cr.openjdk.java.net/~acorn/8009731.3/webrev/
> 
> src/share/vm/classfile/systemDictionary.cpp
> src/share/vm/classfile/systemDictionary.hpp
>    No comments on the above two.
> 
> src/share/vm/interpreter/linkResolver.cpp
>    Line 474 start at the leftmost column:
> 
>    473         char* target =
>    474 InstanceKlass::cast(resolved_method->method_holder())->name()->as_C_string();
> 
>    If you have to break the line due to length, I suggest:
> 
>        char* target = InstanceKlass::cast(resolved_method->method_holder())
>                         ->name()->as_C_string();
> 
> 
> src/share/vm/oops/klassVtable.cpp
>    No comment.
> 
> Dan
> 
> 
>> 
>> Sorry - I am backporting two fixes from my lambda repo - the first to fix the error message and the second
>> to fix the resolved_method for overpasses for the actual loader constraint check and I forgot that the original needed
>> the resolved_method->method_holder() fixed in printing the error message (but the loader constraint check itself
>> is correct today except for overpasses).
>> 
>> The only changes from the last webrev are:
>> 1) linkResolver.cpp - fix target for 2 the two pertinent error messages
>> 2) systemDictionary.hpp - fix alignment as Coleen suggested
>> 
>> thanks,
>> Karen
>> 
>> On Mar 27, 2013, at 8:16 AM, Coleen Phillimore wrote:
>> 
>>> On 3/27/2013 8:11 AM, Karen Kinnear wrote:
>>>> Thank you Dan, David, Coleen and Yumin for your quick reviews.
>>>> I updated the bug to show the before and after.
>>> Final looks good.   In systemDictionary.hpp, do the arguments on the second line line up?   (minor nit - if you change that don't send out another review!!)
>>> 
>>> You can have the queue first if you're ready.
>>> 
>>> thanks,
>>> Coleen
>>>> thanks,
>>>> Karen
>>>> 
>>>> On Mar 26, 2013, at 9:05 PM, Daniel D. Daugherty wrote:
>>>> 
>>>>>> http://cr.openjdk.java.net/~acorn/8009731.2/webrev/
>>>>> src/share/vm/classfile/systemDictionary.cpp
>>>>> src/share/vm/classfile/systemDictionary.hpp
>>>>> src/share/vm/interpreter/linkResolver.cpp
>>>>> src/share/vm/oops/klassVtable.cpp
>>>>>    No comments on any of the above.
>>>>> 
>>>>> Could you add a note to the bug showing the original test output
>>>>> versus the revised test output? There's a lot of commentary in
>>>>> the bug report and a clear statement of "what it was" versus
>>>>> "what it is" will help...
>>>>> 
>>>>> Dan
>>>>> 
>>>>> 
>>>>> On 3/26/13 6:45 PM, Karen Kinnear wrote:
>>>>>> Thank you Coleen and Yumin and Dan. Good catches.
>>>>>> 
>>>>>> I did Dan's suggested changes and included systemDictionary.hpp in the following webrev:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~acorn/8009731.2/webrev/
>>>>>> 
>>>>>> thanks,
>>>>>> Karen
>>>>>> 
>>>>>> On Mar 26, 2013, at 6:56 PM, Daniel D. Daugherty wrote:
>>>>>> 
>>>>>>> On 3/26/13 4:33 PM, Karen Kinnear wrote:
>>>>>>>> Thank you Coleen and David.
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~acorn/8009731.1/webrev/
>>>>>>> Thumbs up.
>>>>>>> 
>>>>>>> src/share/vm/classfile/systemDictionary.cpp
>>>>>>>    2201       Symbol* s = sig_strm.as_symbol(CHECK_NULL);
>>>>>>>    2202       Symbol*  sig  = s;
>>>>>>>        I know you didn't change these lines, but:
>>>>>>> 
>>>>>>>        Local 's' isn't used except to set 'sig'. Is there a reason
>>>>>>>        for the duplicate variable?
>>>>>>> 
>>>>>>>         Feel free to ignore the above if you're pressed for time.
>>>>>>> 
>>>>>>> src/share/vm/interpreter/linkResolver.cpp
>>>>>>>    No comments.
>>>>>>> 
>>>>>>> src/share/vm/oops/klassVtable.cpp
>>>>>>>    No comments.
>>>>>>> 
>>>>>>> Dan
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>>> Problem was introduced with 6990754, changeset 2059.
>>>>>>>> 
>>>>>>>> I like Coleen's solution better - so we don't run the risk of someone "fixing" the apparent need for a ResourceMark
>>>>>>>> in the future. So I changed this to use a Symbol*.
>>>>>>>> 
>>>>>>>> vm.quick.testlist rerun in progress.
>>>>>>>> 
>>>>>>>> thanks,
>>>>>>>> Karen
>>>>>>>> 
>>>>>>>> On Mar 25, 2013, at 8:47 PM, Coleen Phillimore wrote:
>>>>>>>> 
>>>>>>>>> On 3/25/2013 8:10 PM, David Holmes wrote:
>>>>>>>>>> Hi Karen,
>>>>>>>>>> 
>>>>>>>>>> Looks good to me.
>>>>>>>>>> 
>>>>>>>>>> On 26/03/2013 7:08 AM, Karen Kinnear wrote:
>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~acorn/8009731/webrev/
>>>>>>>>>> I can't quite determine why the extra ResourceMark caused the observed symptoms. I would have expected something more dramatic if we returned a string that was already released - are we simply overwriting it with a later class name (hence the wrong name in the message)? Do we also know when this regression was introduced?
>>>>>>>>> So that's why I said "Looks ok" rather than "Looks good."   I think that function should return a Symbol* because it's not obvious to the casual observer that the as_C_string() string is resource allocated and the ResourceMark destructor will reclaim the memory. I think the error message might have been either garbled or pointing to a different string.  It could have been like that for a while and nobody noticed.
>>>>>>>>> 
>>>>>>>>> Coleen
>>>>>>>>> 
>>>>>>>>>>> Bug: https://jbs.oracle.com/bugs/browse/JDK-8009731
>>>>>>>>>> This is not visible external to Oracle.
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>> 
>>>>>>>>>>> Fix loader constraint violation error message.
>>>>>>>>>>> 
>>>>>>>>>>> tests:
>>>>>>>>>>> bug report test
>>>>>>>>>>> vm.quick.testlist in parallel
>>>>>>>>>>> 
>