RFR: 8199220: Zero build broken after 8195103 and 8191102 (was RFR: 8199220: Zero build broken)

[Thomas Stüfe]

On Mon, Mar 12, 2018 at 10:32 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Reminds me of :
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-
> November/029289.html
>
> Could this be the same issue?
>

Hi Edward,

I saw that the mail thread I linked yesterday night was cut off in the
archives - the interesting part was off-list, some more in-depth
explanation by Erik. Since it may be the same issue here, I'll paste Erik's
off-list answer:

<quote>
....
In particular, the problem is that SUPPORTS_NATIVE_CX8 does not seem to be
set on zero, despite actually running on a 64 bit machine.
As a consequence, when you perform atomics, my Access API is trying to be
clever and see if it is compile-time certain that we are not performing
wide atomics with this mechanism:

  // This metafunction returns whether it is possible for a type T to
require


  // locking to support wide atomics or not.



  template <typename T>
#ifdef SUPPORTS_NATIVE_CX8
  struct PossiblyLockedAccess: public IntegralConstant<bool, false> {};
#else



  struct PossiblyLockedAccess: public IntegralConstant<bool, (sizeof(T) >
4)> {};

#endif

What happens in this case with zero is that if we do not have
SUPPORTS_NATIVE_CX8, then it will expand a path where it tries to emulate
wide atomics with a lock. But that wide atomic stuff assumes that we are
really handling a 64 bit integer, *not* an oop. Because oops should *never*
require wide atomics.

The fix for this is either:
1) Make sure SUPPORTS_NATIVE_CX8 is set in globalDefinitions for zero when
running on a 64 bit machine, or
2) Change the metafunction that switches to:

  // This metafunction returns whether it is possible for a type T to
require


  // locking to support wide atomics or not.



  template <typename T>
#ifdef SUPPORTS_NATIVE_CX8
  struct PossiblyLockedAccess: public IntegralConstant<bool, false> {};
#else



  struct PossiblyLockedAccess: public IntegralConstant<bool, (sizeof(T) >
sizeof(void*))> {};

#endif

...so that pointer sized values are never considered for wide atomics.

Arguably, it is really SUPPORTS_NATIVE_CX8 that should be set.

A good trick is to change the ifdefs.

Instead of #ifdef SUPPORTS_NATIVE_CX8, it is better to always explicit
define it to 0 or 1, and then use #if SUPPORTS_NATIVE_CX8 instead. We do
this with e.g. INCLUDE_ALL_GCS Then if it was not defined, the compiler
will die a bit earlier and tell you that you are referring to something
that does not exist, instead of assuming that if it has not been defined,
it means it is not supported.
</quote>

Since we had a number of header file shuffling arounds lately, this may be
a regression and SUPPORTS_NATIVE_CX8 got lost? However, this is just a
hunch - may be a different problem. Beware of wild geese.

Kind Regards, Thomas



>
> On Mon 12. Mar 2018 at 21:49, Edward Nevill <edward.nevill at gmail.com>
> wrote:
>
>> On Mon, 2018-03-12 at 19:54 +0000, Thomas Stüfe wrote:
>> > Hi Edward,
>> >
>> > Thanks a lot for the fixing work!
>> >
>> > However, I am not so sure about the change to debug.hpp. Is the point
>> of the Static assert thing not The missing <false> Specialization? In which
>> case the compile error you saw there was a static assert firing...
>> > I may be wrong, maybe Erik could clarify?
>> >
>> > Otherwise the change looks good. Thank you.
>> >
>>
>> Yes, of course, I see the purpose of the STATIC_ASSERT now. Kind of
>> obvious from the name.
>>
>> The failure is in
>>
>>   template <DecoratorSet decorators, typename T>
>>   static void verify_types(){
>>     // If this fails to compile, then you have sent in something that is
>>     // not recognized as a valid primitive type to a primitive Access
>> function.
>>     STATIC_ASSERT((HasDecorator<decorators,
>> INTERNAL_VALUE_IS_OOP>::value || // oops have already been validated
>>                    (IsPointer<T>::value || IsIntegral<T>::value) ||
>>                     IsFloatingPoint<T>::value)); // not allowed primitive
>> type
>>   }
>>
>> and the error is
>>
>> /home/ed/openjdk/hs/src/hotspot/share/oops/access.inline.hpp: In
>> instantiation of ‘void AccessInternal::verify_types() [with long unsigned
>> int decorators = 4096; T = volatile oop]’:
>>
>> I will continue too look at this but would appreciate some help.
>
>
>>
>> Thanks,
>> Ed.
>>
>>