[PATCH] D70539: [clang][CodeGen] Implicit Conversion Sanitizer: handle increment/derement (PR44054)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: rjmccall, erichkeane, rsmith, vsk.
lebedev.ri added a project: LLVM.
Herald added subscribers: llvm-commits, Sanitizers, cfe-commits, dexonsmith, mehdi_amini.
Herald added projects: clang, Sanitizers.

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 <https://bugs.llvm.org/show_bug.cgi?id=39519>,
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 pessimize 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.
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.
4. OR, we would need to do that in the compiler-rt handler. Which means we'll need a whole new handler. Also doesn't really see all that worthwhile 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.

While looking into this, i have noticed a few more LLVM middle-end missed canonicalizations,
and filed PR44100 <https://bugs.llvm.org/show_bug.cgi?id=44100>,
PR44102 <https://bugs.llvm.org/show_bug.cgi?id=44102>.

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 <https://bugs.llvm.org/show_bug.cgi?id=44054>.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70539

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/catch-implicit-conversions-incdec-basics.c
  clang/test/CodeGen/catch-implicit-integer-arithmetic-value-change-incdec-basics.c
  clang/test/CodeGen/catch-implicit-integer-conversions-incdec-basics.c
  clang/test/CodeGen/catch-implicit-integer-sign-changes-incdec-basics.c
  clang/test/CodeGen/catch-implicit-integer-truncations-incdec-basics.c
  clang/test/CodeGen/catch-implicit-signed-integer-truncations-incdec-basics.c
  clang/test/CodeGen/catch-implicit-signed-integer-truncations-incdec.c
  clang/test/CodeGen/catch-implicit-unsigned-integer-truncations-incdec-basics.c
  compiler-rt/test/ubsan/TestCases/ImplicitConversion/signed-integer-truncation-incdec.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70539.230443.patch
Type: text/x-patch
Size: 68271 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191121/15c46ddf/attachment-0001.bin>