Page MenuHomePhabricator

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

Authored by lebedev.ri on Nov 21 2019, 6:40 AM.

Details

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 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,
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.

Diff Detail

Event Timeline

lebedev.ri created this revision.Nov 21 2019, 6:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 21 2019, 6:40 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 2 others. · View Herald Transcript

I haven't looked at the tests because I don't terribly understand the sanitizer IR (hopefully someone else can take a look), but the logic/motivation seems solid to me.

clang/lib/CodeGen/CGExprScalar.cpp
2429–2473

Should this be 'has' instead of 'hasOneOf'?

2435

comma at the end here isn't required. The one after 'enabled' needs to be a semicolon I think?

Thank you for taking a look!

I haven't looked at the tests because I don't terribly understand the sanitizer IR (hopefully someone else can take a look),

You can ignore the final IR itself in princible. Since this implementation approach avoided re-engineering the checking,
we can piggy-back on the expectation that if the sanitization IR was wrong, we'd catch it already (it is not wrong.).

So only the CGExprScalar.cpp changes need review in principle - the promotion + wide add + demotion.

but the logic/motivation seems solid to me.

Yay! :)

clang/lib/CodeGen/CGExprScalar.cpp
2429–2473

Uhm, it looks it can be CGF.SanOpts.has(SanitizerKind::ImplicitSignedIntegerTruncation),
but then i will need to add a defensive assert. check-clang is running..

2435

comma at the end here isn't required.

Right.

The one after 'enabled' needs to be a semicolon I think?

It doesn't look like that to me?

aaron.ballman added inline comments.Nov 21 2019, 7:38 AM
clang/lib/CodeGen/CGExprScalar.cpp
2431

modelled -> modeled

2435

I'd reword it to make the problem go away. :-D

Because we still want to catch these cases when the sanitizer is enabled, we perform the promotion, then perform the increment/decrement in the wider type, and finally perform the demotion. This will catch lossy demotions.
lebedev.ri marked 7 inline comments as done.

@erichkeane & @aaron.ballman thank you for taking a look.

Addressed review notes.

clang/lib/CodeGen/CGExprScalar.cpp
2429–2473

So, this actually showed me a bug - we should also check for ImplicitIntegerSignChange.

erichkeane accepted this revision.Nov 25 2019, 8:03 AM
This revision is now accepted and ready to land.Nov 25 2019, 8:03 AM

Thank you for the review!
I'll hold this off for a bit in case anyone else wants to comment.

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Nov 27 2019, 6:08 AM

Temporarily reverted in rG62098354ab1c6c0dd2cecc8ac526d411bfe8aa20 - the assertion does not hold, need to remove it likely.

This revision is now accepted and ready to land.Nov 27 2019, 6:08 AM
This revision was automatically updated to reflect the committed changes.