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

Vedant Kumar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 11 15:48:24 PDT 2018


vsk added a subscriber: klimek.
vsk added inline comments.


================
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();
----------------
lebedev.ri wrote:
> vsk wrote:
> > lebedev.ri wrote:
> > > 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..
> > As for (1), it's not necessary to push/pop the stack when this sanitizer is disabled. And for (2), IIUC the only interesting case is "explicit-cast <implicit-cast>+", and none of the implicit casts here have more than one parent.
> > I think maintaining a stack of visited cast exprs in the emitter be cheaper/simpler than using ASTContext::getParents. 
> 
> {F6660377}
> 
> So yeah, this could work.
> Except sadly it breaks in subtle cases like https://godbolt.org/g/5V2czU
> I have added those tests beforehand.
> 
> Is `ASTContext::getParents()` really horribly slow so we want to duplicate/maintain/track
> the current AST stack ourselves?
> If so, we will need to maintain the *entire* stack, not just `CastExpr''s...
I think the scan in 'IsTopCastPartOfExplicitCast' can be fixed: while traversing backwards, you'd need to check that the previously-visited cast expr is the child of the current expr. That should address the false negative you pointed out in interference1. I don't yet see what the issue is with interference0.

Could you explain why maintaining a stack of unfinished casts wouldn't work? I don't understand why you'd need the entire stack. My sense is that it's not required to match the "explicit-cast <implicit-cast>+" pattern, but I could easily be missing something here. As for why this might be worth looking into, I think scanning for an explicit cast is much easier to understand when working with a stack.

+ @klimek to comment on what to expect in terms of the overhead of ASTContext::getParents. Regardless of what approach we pick, it would help to see pre/post-patch compile times for a stage2 build of something like clang or llc.


Repository:
  rC Clang

https://reviews.llvm.org/D48958





More information about the cfe-commits mailing list