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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The overall approach here seems reasonable.  I mean, technically the undefined behavior is happening in the library, but detecting it early seems like a good idea.

This approach does have a significant limitation, though: CGBuiltin won't detect cases that involve taking the address of abs().  So ubsan won't end up picking up undefined behavior in such calls, and -fwrapv won't apply.  Maybe that's rare enough we don't really care, though.

Needs a release note.

Do we want to update ubsan documentation to reflect this?



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697
+    bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin);
+    bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow);
+
----------------
Putting the behavior under both "builtin" and "signed-integer-overflow" feels a little weird; is there any precedent for that?


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


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

https://reviews.llvm.org/D156821