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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
