[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

[Artem Labazov via Phabricator via cfe-commits]

artem added a comment.

Thanks for the feedback! I will resolve the problems in the next revision after some clarifications.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697
+    bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin);
+    bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow);
+
----------------
efriedma wrote:
> Putting the behavior under both "builtin" and "signed-integer-overflow" feels a little weird; is there any precedent for that?
The problem is, we are instrumenting a builtin function, so on the one hand it is reasonable to be controlled by `-fsanitize=builtin`. On the other hand, GCC treats abs() as an arithmetic operation, so it is being instrumented by `-fsanitize=signed-integer-overflow` (`abs(INT_MIN)` causes a negation overflow).

I have decided to enable instrumentation under any of the flags, but I am not sure whether it is the right choice.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2702
+    case LangOptions::SOB_Defined:
+      Result = Builder.CreateBinaryIntrinsic(
+          Intrinsic::abs, EmitScalarExpr(E->getArg(0)), Builder.getFalse(),
----------------
efriedma wrote:
> Can we land the change to directly generate calls to llvm.abs() in the default case separately? This might end up impacting generated code for a variety of workloads, and I'd prefer to have a more clear bisect point.
I used llvm.abs() for code simplicity, since middle-end combines the instructions to it anyways. I think this part can be dropped entirely because the intrinsic is not the best possible option either (the compiler emits conditional move and fails to elide extra sign checks if the argument is known to be non-negative).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156821/new/

https://reviews.llvm.org/D156821