Request for review: 8012695: Assertion message displays %u and %s text instead of actual values

[Coleen Phillimore]

On 4/23/2013 10:39 AM, Coleen Phillimore wrote:
>
> On 04/23/2013 10:05 AM, harold seigel wrote:
>> Hi,
>>
>> Thanks for the comments.  I would prefer to leave the change as is 
>> because it is small and only affects assertions.  Making the 
>> suggested change would require changing all 30+ calls to 
>> check_property() and require additional testing.  Also, not all calls 
>> to check_property() end in "in class file %s".  So additional changes 
>> would be needed for those calls.
>>
>> I don't think the additional work is worth the benefit. However, I 
>> could open another bug about cleaning up calls to check_property().
>>
>> I also don't think the check for _class_name != NULL is needed 
>> because the code that dereferences _class_name is ASSERT only and 
>> wouldn't affect users.
>
> Yabut, you really want the assert to fire rather than a crash in the 
> assert statement!

Harold pointed out to me that _class_name is used in about 32 other 
check_property and assert messages, not just this one, so this is 
fine.   Ship it.
Coleen

> Coleen
>
>>
>> Thanks, Harold
>>
>>
>> On 4/22/2013 2:21 PM, Coleen Phillimore wrote:
>>>
>>> This sounds like a nice cleanup actually.  I don't know how you 
>>> (Harold) feel about growing this change to do the cleanup, as well 
>>> as fixing the problem.
>>>
>>> One thing that I think would be safe is to test that _class_name != 
>>> NULL first.  It should never be null but you never know.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 04/22/2013 01:52 PM, Ioi Lam wrote:
>>>> It seems tedious that all callers of check_property have to pass in 
>>>> "...  in class file %s".
>>>>
>>>> Also, if the "%s" is placed at the wrong place we could potentially 
>>>> crash inside printf().
>>>>
>>>> How about this?
>>>>
>>>> check_property(
>>>>     ...
>>>>       "outer_class_info_index %u has bad constant type in class 
>>>> file %s", outer_class_info_index ...)
>>>>
>>>> ->
>>>>
>>>> check_property(
>>>>     ...
>>>>       "outer_class_info_index",  outer_class_info_index, "has bad 
>>>> constant type", ...)
>>>>
>>>> This way the passed-in strings are treated as plain strings instead 
>>>> of printf formatters.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 04/22/2013 06:49 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this small fix for bug 8012695.
>>>>>
>>>>> Summary:   The fix uses err_msg() to create an assertion message 
>>>>> containing actual text instead of printf formatters.
>>>>>
>>>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8012695/ 
>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8012695/>
>>>>>
>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8012695
>>>>>
>>>>> Regression testing was done using JCK Lang and VM tests and ute 
>>>>> vm.quick.testlist tests.  Also, assertion messages were generated 
>>>>> to ensure that the fix works:
>>>>>
>>>>> Old message:   # fatal error: Invalid field attribute index %u in 
>>>>> class file %s
>>>>>
>>>>> New message:  #  fatal error: Invalid code attribute name index 92 
>>>>> in class file vm/mlvm/anonloader/share/AnonkTestee01
>>>>>
>>>>> Thank you!
>>>>> Harold
>>>>
>>>
>>
>