RFR: 8145096: Undefined behaviour in HotSpot

[Kim Barrett]

On Dec 10, 2015, at 8:30 AM, Andrew Haley <aph at redhat.com> wrote:
> 
> I've been tracing through HotSpot with GCC's undefined behaviour
> sanitizer

Nice!

I've only skimmed the changes in the opto directory.  You should get
compiler people to review those changes.

> This patch introduces some functions which perform java-like
> arithmetic: java-add, etc.  There is no perfectly portable way to do
> this in C++,

I disagree; see below.

> but one way which is widely supported is known as the
> "union trick": put the signed integers in a union with their unsigned
> equivalents, do the arithmetic, and return the signed versions.  The
> "obvious" way to do this via casts does not work with GCC and probably
> does not work with other compilers either.  The union trick is well-
> supported by C++ compilers and generates efficient code.  I believe
> that we should be able to use it everywhere.
> 
> Apart from the undefined behaviour being fixed, this patch should
> cause no behavioural changes, except in one case.
> AdvancedThresholdPolicy::weight() grossly overflows, so much so that
> its result is substantially noise. That's fixed here.

Nice find.

> http://cr.openjdk.java.net/~aph/8145096-1/
> 

------------------------------------------------------------------------------ 
src/os/posix/vm/os_posix.cpp
 756     unsigned int i;

I'm not sure what the purpose of changing this to unsigned might be.
The sa_flags member of struct sigaction is of type int.

------------------------------------------------------------------------------
 136   return (double)(method->rate() + 1) * ((double)(method->invocation_count() + 1) * (method->backedge_count() + 1));

I suspect the problem encountered here is that the multiplication of
the two count values can overflow.  If that multiplication was not
grouped, but instead the rate term and invocation count were
multiplied, then multiplied by the backedge count (e.g. use the
natural grouping for operator*), the result should be mathematically
the same, and I think should not run into overflow problems.  Recall
that method->rate() returns float.

So I think a simpler change here might be

  return (method->rate() + 1) * (method->invocation_count() + 1) * (method->backedge_count() + 1);

i.e. remove the parens that group the count multiplication before
multiplying by the rate.

------------------------------------------------------------------------------ 
src/share/vm/utilities/globalDefinitions.hpp
1425 #define JAVA_INTEGER_OP(OP, NAME, TYPE, UNSIGNED_TYPE)  \
1426 static inline TYPE NAME (TYPE in1, TYPE in2) {          \
1427   union { TYPE n1; UNSIGNED_TYPE u1; };                 \
1428   union { TYPE n2; UNSIGNED_TYPE u2; };                 \
1429   n1 = in1; n2 = in2;                                   \
1430   u1 OP ## = u2;                                        \
1431   return n1;                                            \
1432 }

e.g. using the "union trick" for type punning.

To me, this seems to be trading one form of undefined behavior for
another.  Since there *is* a portable solution, which I understand is
also well supported / optimized by compilers, I'd prefer that.

#define JAVA_INTEGER_OP(OP, NAME, TYPE, UNSIGNED_TYPE)  \
inline TYPE NAME (TYPE in1, TYPE in2) {                 \
  STATIC_ASSERT(sizeof(TYPE) == sizeof(UNSIGNED_TYPE)); \
  UNSIGNED_TYPE ures = static_cast<UNSIGNED_TYPE>(in1); \
  ures OP ## = static_cast<UNSIGNED_TYPE>(in2);         \
  TYPE res;                                             \
  memcpy(&res, &ures, sizeof(res));                     \
  return res;                                           \
}

gcc4.8 with -O generates exactly the code one would like for this.
(And unsurprisingly generates pretty horrible code with -O0.)

I'm collecting information about other platforms that I have access to
or know someone who does.

The casts to the unsigned type here are portable.  What would not be
portable is a cast of ures to the signed type, which has
implementation-defined behavior (and *not* undefined behavior) for
some values.  So I was surprised by the statement

  The "obvious" way to do this via casts does not work with GCC ...

when the gcc documentation says
https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Integers-implementation.html#Integers-implementation

  4.5 Integers

  For conversion to a type of width N, the value is reduced modulo 2^N
  to be within range of the type; no signal is raised.

Of course, one shouldn't rely on this in portable code.

Note that I would not like to see these java_xxx functions used as a
general "solution" to overflow problems.  Most cases of signed integer
arithmetic overflow are actual bugs, and papering over them with these
functions would be a mistake.  I haven't reviewed them carefully, but
the way these are being used in the various changes in the opto
directory look appropriate, in order to match Java's defined behavior.
But I wouldn't be surprised if there were other places (like the
problem found in advancedThresholdPolicy.cpp) where I would not want
these operations used.  In fact, I'd like the comment describing these
operations to say something along that line.

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