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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
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);
----------------
vsk wrote:
> 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.
The number of overloads is indeed unwieldy.


================
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();
----------------
vsk wrote:
> 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.
Hmm, two things come to mind:
1. This pessimizes the (most popular) case when the sanitizer is disabled.
2. `ASTContext::getParents()` may return more than one parent. I'm not sure if that matters here?
I'll take a look..


Repository:
  rC Clang

https://reviews.llvm.org/D48958