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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697
+    bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin);
+    bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow);
+
----------------
artem wrote:
> 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.
I'd prefer to just follow gcc here, I think, if there isn't any strong reason to pick a different approach.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2702
+    case LangOptions::SOB_Defined:
+      Result = Builder.CreateBinaryIntrinsic(
+          Intrinsic::abs, EmitScalarExpr(E->getArg(0)), Builder.getFalse(),
----------------
artem wrote:
> 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).
Sure, that works too.


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

https://reviews.llvm.org/D156821