[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Thanks for working on this!



================
Comment at: docs/ReleaseNotes.rst:277
+  behaviour. But they are not *always* intentional, and are somewhat hard to
+  track down.
 
----------------
Could you mention whether the group is enabled by -fsanitize=undefined?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:318
 
-  Value *EmitScalarConversion(Value *Src, QualType SrcTy, QualType DstTy,
+  Value *EmitScalarConversion(Value *Src, QualType SrcType, QualType DstType,
                               SourceLocation Loc, bool TreatBooleanAsSigned);
----------------
I think the number of overloads here is really unwieldy. There should be a simpler way to structure this. What about consolidating all four overloads into one? Maybe:

```
struct ScalarConversionsOpts {
  bool TreatBoolAsUnsigned = false;
  bool EmitImplicitIntegerTruncationCheck = false;
};

Value *EmitScalarConversion(Src, SrcTy, DstTy, Loc, Opts = ScalarConversionOpts())
```

It's not necessary to pass CastExpr in, right? There's only one place where that's done. It seems simpler to just do the SanOpts / isCastPartOfExplicitCast checking there.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:951
+//   implicit cast -> explicit cast                 <- that is an explicit cast.
+static bool CastIsPartOfExplictCast(ASTContext &Ctx, const Expr *E) {
+  // If it's a nulllptr, or not a cast, then it's not a part of Explict Cast.
----------------
nit, function names typically begin with a verb: 'isCastPartOf...'


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();
----------------
I think maintaining a stack of visited cast exprs in the emitter be cheaper/simpler than using ASTContext::getParents. You could push CE here and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check then simplifies to a quick stack traversal.


Repository:
  rC Clang

https://reviews.llvm.org/D48958