review (S) for 7032162: assert(flat != TypePtr::BOTTOM) failed: cannot alias-analyze an untyped ptr
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Apr 28 14:37:37 PDT 2011
Good.
Vladimir
Tom Rodriguez wrote:
> The push for this failed just before my vacation and I didn't have time to investigate then. It turns out that the klass() call needs to guard against the null object so it now looks like this:
>
> diff -r 149bb459be66 src/share/vm/ci/ciObject.cpp
> --- a/src/share/vm/ci/ciObject.cpp
> +++ b/src/share/vm/ci/ciObject.cpp
> @@ -194,6 +194,16 @@
> // ciObject::should_be_constant()
> bool ciObject::should_be_constant() {
> if (ScavengeRootsInCode >= 2) return true; // force everybody to be a constant
> + if (!JavaObjectsInPerm && !is_null_object()) {
> + // We want Strings and Classes to be embeddable by default since
> + // they used to be in the perm world. Not all Strings used to be
> + // embeddable but there's no easy way to distinguish the interned
> + // from the regulars ones so just treat them all that way.
> + ciEnv* env = CURRENT_ENV;
> + if (klass() == env->String_klass() || klass() == env->Class_klass()) {
> + return true;
> + }
> + }
> return handle() == NULL || !is_scavengable();
> }
>
> tom
>
> On Apr 6, 2011, at 3:52 PM, Tom Rodriguez wrote:
>
>> Thanks!
>>
>> tom
>>
>> On Apr 6, 2011, at 2:07 PM, Vladimir Kozlov wrote:
>>
>>> This looks better.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> On Apr 6, 2011, at 1:49 PM, Vladimir Kozlov wrote:
>>>>> You replaced 'require_constant' with 'true' to the call make_from_constant() so I was wondering if it may change behavior. Specifically it may produce NULL now
>>>>> if oop_constant->can_be_constant() is 'false':
>>>>>
>>>>> if (require_constant) {
>>>>> if (!o->can_be_constant()) return NULL;
>>>> In the !JavaObjectInPerm world Strings should always return true for can_be_constant. Actually I think it would be better to fix it by changing ciObject::should_be_constant instead of just patching that use. I also included Classes in the test for consistency with the old behaviour. So I've removed the changes in parse3.cpp and done this instead.
>>>> diff -r 8e77e1f26188 src/share/vm/ci/ciObject.cpp --- a/src/share/vm/ci/ciObject.cpp +++ b/src/share/vm/ci/ciObject.cpp @@ -194,6 +194,16 @@ bool ciObject::can_be_constant() {
>>>> // ciObject::should_be_constant() bool ciObject::should_be_constant() { if (ScavengeRootsInCode >= 2) return true; // force everybody to be a constant + if (!JavaObjectsInPerm) { + // We want Strings and Classes to be embeddable by default since + // they used to be in the perm world. Not all Strings used to be + // embeddable but there's no easy way to distinguish the interned
+ // from the regulars ones so just treat them all that way. + ciEnv* env = CURRENT_ENV; + if (klass() == env->String_klass() || klass() == env->Class_klass()) { + return true; + } + } return handle() == NULL || !is_
scavengable(); }
>>>> tom
>>>>> Vladimir
>>>>>
>>>>> Tom Rodriguez wrote:
>>>>>> http://cr.openjdk.java.net/~never/7032162
>>>>>> 7032162: assert(flat != TypePtr::BOTTOM) failed: cannot alias-analyze an untyped ptr
>>>>>> Reviewed-by:
>>>>>> This appears to be an existing problem with OptimizeStringConcat where
>>>>>> missing control can allow some loads to be split up to where their
>>>>>> base is NULL which causes addresses that can't be analyzed. The fix
>>>>>> is to use the appropriate control. If it's truly unneeded it will be
>>>>>> optimized away. The bug was exposed by the String not in perm changes
>>>>>> because we were no longer treating some constant strings as embeddable
>>>>>> constants. This is fixed by handling them specially in push_constant.
>>>>>> Tested with failing test from report and full CTW with string opts.
>
More information about the hotspot-compiler-dev
mailing list