HomePhabricator

[clang][CodeGen] Implicit Conversion Sanitizer: handle increment/decrement…

Authored by lebedev.ri on Nov 27 2019, 2:04 AM.

Description

[clang][CodeGen] Implicit Conversion Sanitizer: handle increment/decrement (PR44054)

Summary:
Implicit Conversion Sanitizer is *almost* feature complete.
There aren't *that* much unsanitized things left,
two major ones are increment/decrement (this patch) and bit fields.

As it was discussed in
PR39519,
unlike CompoundAssignOperator (which is promoted internally),
or BinaryOperator (for which we always have promotion/demotion in AST)
or parts of UnaryOperator (we have promotion/demotion but only for
certain operations), for inc/dec, clang omits promotion/demotion
altogether, under as-if rule.

This is technically correct: https://rise4fun.com/Alive/zPgD
As it can be seen in InstCombineCasts.cpp canEvaluateTruncated(),
add/sub/mul/and/or/xor operators can all arbitrarily
be extended or truncated:
https://github.com/llvm/llvm-project/blob/901cd3b3f62d0c700e5d2c3f97eff97d634bec5e/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L1320-L1334

But that has serious implications:

  1. Since we no longer model implicit casts, do we pessimise their AST representation and everything that uses it?
  2. There is no demotion, so lossy demotion sanitizer does not trigger :]

Now, i'm not going to argue about the first problem here,
but the second one needs to be addressed. As it was stated
in the report, this is done intentionally, so changing
this in all modes would be considered a penalization/regression.
Which means, the sanitization-less codegen must not be altered.

It was also suggested to not change the sanitized codegen
to the one with demotion, but i quite strongly believe
that will not be the wise choice here:

  1. One will need to re-engineer the check that the inc/dec was lossy in terms of @llvm.{u,s}{add,sub}.with.overflow builtins
  2. We will still need to compute the result we would lossily demote. (i.e. the result of wide addition/subtraction)
  3. I suspect it would need to be done right here, in sanitization. Which kinda defeats the point of using @llvm.{u,s}{add,sub}.with.overflow builtins: we'd have two adds with basically the same arguments, one of which is used for check+error-less codepath and other one for the error reporting. That seems worse than a single wide op+check.
  4. OR, we would need to do that in the compiler-rt handler. Which means we'll need a whole new handler. But then what about the CompoundAssignOperator, it would also be applicable for it. So this also doesn't really seem like the right path to me.
  5. At least X86 (but likely others) pessimizes all sub-i32 operations (due to partial register stalls), so even if we avoid promotion+demotion, the computations will likely be performed in i32 anyways.

So i'm not really seeing much benefit of
not doing the straight-forward thing.

While looking into this, i have noticed a few more LLVM middle-end
missed canonicalizations, and filed
PR44100,
PR44102.

Those are not specific to inc/dec, we also have them for
CompoundAssignOperator, and it can happen for normal arithmetics, too.
But if we take some other path in the patch, it will not be applicable
here, and we will have most likely played ourselves.

TLDR: front-end should emit canonical, easy-to-optimize yet
un-optimized code. It is middle-end's job to make it optimal.

I'm really hoping reviewers agree with my personal assessment
of the path this patch should take..

Fixes PR44054.

Reviewers: rjmccall, erichkeane, rsmith, vsk

Reviewed By: erichkeane

Subscribers: mehdi_amini, dexonsmith, cfe-commits, #sanitizers, llvm-commits, aaron.ballman, t.p.northover, efriedma, regehr

Tags: #llvm, #clang, #sanitizers

Differential Revision: https://reviews.llvm.org/D70539