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

[harold seigel]

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.

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
>>
>