[12] RFR: 8214059: Undefined behaviour in ADLC

[Aleksey Shipilev]

On 11/19/18 10:06 PM, Simon Tooke wrote:
> Hello,
> 
> I tried compiling with -fsanitize=undefined, and it found some issues in
> ADLC, relying on undefined behaviour.
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-8214059
> Fix:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/stooke/JDK-8214059/01/webrev/
> 
> There are two issues: an incorrect C-style downcast (fixed by casting to
> the correct class),

I wonder if this should instead be:

   while ((param = inst._parameters.iter()) != NULL) {
-    OperandForm* opForm = (OperandForm*) inst._localNames[param];
+    OpClassForm* opForm = inst._localNames[param]->is_opclass();
+    assert(opForm != NULL, "sanity");
     encoding->add_parameter(opForm->_ident, param);
   }


> and a reliance on undefined integer overflow
> behaviour (proposed fix by using long long arithmetic and a compile-time
> guard to check correct sizeof())

So, wait. Why do we redefine STATIC_ASSERT? Should we instead make the globally available
STATIC_ASSERT right if it has problems?

I think the canonical way to perform overflow-sensitive addition involves checking against INT_MAX.
Or, in this case, Expr::Max. Instead of this "long long" and STATIC_ASSERT mess, we can say:

int Expr::compute_max(const Expr *c1, const Expr *c2) {
  int v1 = c1->_max_value;
  int v2 = c2->_max_value;

  // Check for overflow without producing UB. If v2 is positive
  // and not larger than Max, the subtraction cannot underflow.
  assert(0 <= v2 && v2 <= Expr::Max, "sanity");
  if (v1 > Expr::Max - v2) {
    return Expr::Max;
  }

  return v1 + v2;
}

Thanks,
-Aleksey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20181119/fce2678c/signature.asc>