RFR(S): 8029351: assert(bt != T_OBJECT) failed: Guard is incorrect in VM:defmeth

[Coleen Phillimore]

On 12/9/2013 9:17 AM, David Chase wrote:
> I was attempting Christian's cleanup, and saw what would have been a nice one
>
>    tag. basic_type() == T_OBJECT
>
> but that did not evaluate the same condition -- it also includes
>
>    is_klass_index() || is_string_index()
>
> which is not in the generateOopMap code.  Is it wrong to include those, or did some Bright Boy
> happen to know that he could save a few cycles by omitting those cases (and thus burn how many
> beeelions of CPU cycles on testing etc as we wonder at his undocumented intent?)
>
> bool is_object() const            { return is_klass() || is_unresolved_klass() ||
>                                        is_string() || is_method_handle() || is_method_type() ||
>                                        is_method_handle_in_error() || is_method_type_in_error(); }
>    // But NOT is_klass_index or is_string_index, else we could use basic_type() == T_OBJECT?

These tags are temporary during class file parsing, so omitting them is 
safe (famous last words).
Coleen

>
> Any idea?
>
> David
>
> On 2013-12-09, at 8:54 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>
>> This looks good.   I don't know why I decided that these weren't needed when I went through all of these with my previous bug fix, but apparently they are.
>> Thanks for fixing these.
>> Coleen
>>
>> On 12/7/2013 10:12 PM, David Chase wrote:
>>> RT-dev CC'd because this was more of a runtime bug, except that involved methodHandles.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8029351
>>> webrev: http://cr.openjdk.java.net/~drchase/8029351/webrev.01/
>>>
>>> Running "defmeth", especially with -Xcomp and -XX:+AggressiveOpts, sometimes (1 in 25 to 1 in 50)
>>> causes a crash in fastdebug, that in release builds is instead a signal-ignoring infinite loop.
>>>
>>> The cause was failure to include the method_handle_in_error and method_type_in_error
>>> cases in the "is this an object?" test.  This immediately led to a blown assertion in a debug build.
>>> Fix was to add the obvious clauses to the if:
>>>
>>>         tag.is_unresolved_klass() ||
>>>         tag.is_string() ||
>>>         tag.is_method_handle() ||
>>> -      tag.is_method_type()) {
>>> +      tag.is_method_type() ||
>>> +      tag.is_method_handle_in_error() ||
>>> +      tag.is_method_type_in_error()) {
>>>       assert(bt == T_OBJECT, "Guard is incorrect");
>>>       cts = CellTypeState::make_line_ref(bci);
>>>     } else {
>>>
>>> Testing:
>>> defmeth, fastdebug build with bug-provoking flags , 1000 times.
>>> All ran w/o crash (i.e., only with other existing known failures), all produced the same output.
>>>
>>> David
>>>