RFR: 8279143: Undefined behaviours in globalDefinitions.hpp [v4]

[Andrew Haley]

On Wed, 5 Jan 2022 01:17:53 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into undefinedBehaviour
>>  - update copyright
>>  - typo
>>  - clean
>>  - Merge branch 'master' into undefinedBehaviour
>>  - Merge branch 'master' of github.com:MeryKitty/jdk into undefinedBehaviour
>>  - implementation limits
>>  - const reference
>>  - words not need to be initialized
>>  - undefined behaviour in globalDefinitions.hpp
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 617:
> 
>> 615:                 std::is_trivially_copy_assignable<T>(), "implementation limits");
>> 616:   T to;
>> 617:   memcpy(&to, &from, sizeof(T));
> 
> During the review of JDK-8145096 it was found that some compilers produce
> wretched code for these kinds of memcpy uses, even at fairly high optimization
> levels.  (I don't know if we still care about those compilers.  Unfortunately I
> don't remember which ones they were, other than gcc/clang/VS all being good.)
> 
> While using the so-called "union trick" is technically undefined behavior, it
> is a technique that is known to be widely and well supported and produces good
> code, at least for the cases where it is being used in HotSpot. In some cases,
> such as gcc (and I think Visual Studio, though can't find a reference right
> now), this behavior is documented.
> 
> Rather than adding a partial bit_cast (or moving it from elsewhere), we should
> be using our existing PrimitiveConversions::cast
> (metaprogramming/primitiveConversions.hpp). That has the small difficulty of a
> circular include dependency with globalDefintions.hpp. That can be fixed by
> moving the various jfoo_cast functions elsewhere (either to
> primitiveConversions.hpp or to a new file; I might prefer the latter (along
> with the Translate<jfloat/jdouble> specializations in primitiveConversions),
> moving these relatively infrequently used utilities to their own dedicated
> location). That also reduces the content of globalDefinitions.hpp, which IMO
> is too much of a random dumping ground.

>From replies to what must by not be a GCC FAQ, type punning through a union like this is explicitly permitted:


inline jlong jlong_cast (jdouble x) {
  DoubleLongConv u;
  u.x = x;
  return u.l;
}

But this isn't:


inline jlong jlong_cast (jdouble x) {
  return ((DoubleLongConv*)&x)->l;
}

Not that it matters to HotSpot, because we use `-fno-strict-aliasing`.

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

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