RFR: 8299125: UnifiedOopRef in JFR leakprofiler should treat thread local oops correctly [v2]

[Stefan Karlsson]

On Tue, 3 Jan 2023 14:28:07 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/share/jfr/leakprofiler/utilities/unifiedOopRef.inline.hpp line 70:
>> 
>>> 68:   LP64_ONLY(assert(((reinterpret_cast<uintptr_t>(ref)) & (1ull << 63)) == 0, "Unexpected high-order bit"));
>>> 69:   UnifiedOopRef result = { (reinterpret_cast<uintptr_t>(ref) LP64_ONLY(<< 1)) | tag };
>>> 70:   assert(result.addr<T>() == ref, "sanity");
>> 
>> This is crazy hard to read.
>
> Yeah. Refactoring out `reinterpret_cast<uintptr_t>(ref)` into a variable might help a bit at least. The assert just want to check that the lowest two bits are not set (the tagging does not destroy information) and that on 64 bit platforms the highest bit is not set (the shift does not destroy information).
> 
> Not sure if there is a better way to express those two invariants.

Maybe it would be easier to read the code if it was rewritten as (uncompiled):

  uintptr_t value = reinterpret_cast<uintptr_t>(ref);

#ifdef _LP64
  // tag_mask is 3 bits. When ref is a narrowOop* we only have 2 alignment
  // bits, because of the 4 byte alignment of compressed oops addresses.
  // Shift up to make way for one more bit.
  assert((value & (1ull << 63)) == 0, "Unexpected high-order bit");
  value <<= 1;
#endif

  UnifiedOopRef result = { value | tag };
  assert(result.addr<T>() == ref, "sanity");

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

PR: https://git.openjdk.org/jdk/pull/11741