RFR: 8263721: Unify oop casting [v2]

[Kim Barrett]

On Tue, 23 Mar 2021 10:18:06 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> In fastdebug builds we replace the "oopDesc* to oop" typedef with a wrapper class that holds an oopDesc*. This wrapper class allows any kind of pointer to be implicitly converted to an oop. So you can write code like this:
>> 
>>   Metadata* m = (Metadata*)0x123;
>>   oop o = m;
>> 
>> and the compiler will unfortunately accept it. Fortunately, this will be caught in release builds, because you can't convert a Method* into an oopDesc*.
>> 
>> One interesting thing is that you can't convert values of integral type too oops:
>> 
>>   uintptr_t m = uintptr_t(123);
>>   oop o = m;
>> 
>> This fails in both fastdebug and release builds. To be able to convert integral values to oops, there are two helper functions:
>> 
>> // For CHECK_UNHANDLED_OOPS, it is ambiguous C++ behavior to have the oop
>> // structure contain explicit user defined conversions of both numerical
>> // and pointer type. Define inline methods to provide the numerical conversions.
>> template <class T> inline oop cast_to_oop(T value) {
>>   return (oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value));
>> }
>> template <class T> inline T cast_from_oop(oop o) {
>>   return (T)(CHECK_UNHANDLED_OOPS_ONLY((oopDesc*))o);
>> }
>> 
>> So, the above example would have to be written as:
>> 
>>   uintptr_t m = uintptr_t(123);
>>   oop o = cast_to_oop(m);
>> 
>> My proposal is that we stop allowing implicit (and explicit) casts from void*, and instead use cast_to_oop whenever we want to cast to oops. We would still allow oopDesc* to be implicitly converted to oop. This would also allow NULL to be converted too oop without casting:
>> 
>>   oop o = NULL;
>> 
>> This will make the code to convert oops a little bit longer. It could be argued that that's a good thing, because everyone should be cautious about converting things into oops. This will also give us one entry-point where we could add (probably temporary) verification code.
>> 
>> An alternative to the suggestion above, could be to completely get rid of cast_to_oop and cast_from_oop. But for that to work we need to stop using NULL, which is an integral 0, and start to use nullptr for oops. I've prototyped this as well, but initial investigations showed that some tended to prefer having the cast_to_oop function. (We could still move from NULL to nullptr, if we think that is a good idea).
>
> Stefan Karlsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Remove casts in merged changes
>  - Merge remote-tracking branch 'origin/master' into 8263721_unify_oop_casting
>  - Convert tab to spaces
>  - Merge remote-tracking branch 'origin/master' into 8263721_unify_oop_casting
>  - Stricter oop casts

Looks good.  Just the one minor nit.

There are a lot of casts to `void*` as an argument that ought to be cleaned up.  And the set of valid types for the cast functions ought to be restricted to "reasonable" types.  But that can be a followup.

src/hotspot/share/oops/oopsHierarchy.hpp line 89:

> 87: 
> 88: public:
> 89:   oop()             : _o(NULL) { register_if_checking(); }

NULL -> nullptr

-------------

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3047