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

[Coleen Phillimore]

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