RFR (S): 7195816: NPG: Crash in c1_ValueType - ShouldNotReachHere

[Coleen Phillimore]

Roland,
I think the cleanups are good.   Additional question about my favorite snippet of code in this webrev:

1038     case T_ARRAY:   // fall through
1039     case T_OBJECT:  // fall through
1040 #ifndef _LP64
1041       // We get here to store a method pointer to the stack to pass to
1042       // a dtrace runtime call. This can't work on 64 bit with
1043       // compressed klass ptrs: T_METADATA can be a compressed klass
1044       // ptr or a 64 bit method pointer.
1045     case T_METADATA:
1046 #endif
1047       if (UseCompressedOops&&  !wide) {
1048         __ movl(as_Address(to_addr), compressed_src);
1049       } else {
1050         __ movptr(as_Address(to_addr), src->as_register());
1051       }
1052       break;


So is this correct also?

1038     case T_ARRAY:   // fall through
1039     case T_OBJECT:  // fall through
1047       if (UseCompressedOops&&  !wide) {
1048         __ movl(as_Address(to_addr), compressed_src);
1049       } else {
1050         __ movptr(as_Address(to_addr), src->as_register());
1051       }
1052       break;
          case T_METADATA:
1041       // We get here to store a method pointer to the stack to pass to
1042       // a dtrace runtime call. This can't work on 64 bit with
1043       // compressed klass ptrs: T_METADATA can be a compressed klass
1044       // ptr or a 64 bit method pointer.
1045       LP64_ONLY(ShouldNotReachHere);
1050       __ movptr(as_Address(to_addr), src->as_register());
            break;


Thanks,
Coleen
On 9/10/2012 9:45 AM, Roland Westrelin wrote:
> Thanks for taking a look at this.
>
>> Thank you for fixing c1.  We didn't realize there was this much left out.
> I took the opportunity to clean things up a bit. The minimal fix for this particular problem could maybe have been smaller but I think the code is better that way and could help catch problems in the future.
>
>>   In c1_LIRAssembler_x86.cpp,  this is sort of confusing.   I would prefer a separate case for C1_METADATA, because it would use UseCompressedKlassPointers rather than UseCompressedOops so even without the ifdef, part of this case doesn't make sense.    Do you have fixes for this in your UseCompressedKlassPointer changes?
> A metadata ptr manipulated by c1 can be either a klass ptr or a method ptr. When the code is emitted in c1_LIRAssembler_*.cpp, we know something is a metadata ptr but the extra bit of information that tells whether it's a method or klass ptr is lost. The code below is the single place where the fact that we don't know whether the ptr is a klass or a method could be a problem: a metadata ptr is written to memory but on 64 bit with compressed klass ptrs it could be either a 32 bit store or a 64 bit store. Anyway, this code path is only taken on 32 bit because the code that C1 generates and the 32 bit x86 C calling convention are such that the only case where a metadata ptr is written to memory is to write it to the stack to pass as an argument to a dtrace runtime call.
>
> This code doesn't change with the UseCompressedKlassPointer changes.
>
> In short it's a corner case that only happens on x86/32 bit.
>
>> Why isn't there the same sort of change from c1_LIRAssembler_sparc.cpp?
> All arguments to dtrace runtime calls are passed in registers so we won't try to write one of them to memory.
>
> Roland.