[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: lib/CodeGen/CodeGenFunction.h:380
+  /// True if sanitizer checks for current pointer value are generated.
+  bool PointerChecksAreEmitted = false;
+
----------------
rjmccall wrote:
> sepavloff wrote:
> > rjmccall wrote:
> > > I don't think this is a good way of doing this.  Using global state makes this unnecessarily difficult to reason about and can have unintended effects.  For example, you are unintentionally suppressing any checks that would be done as part of e.g. evaluating default arguments to the default constructor.
> > > 
> > > You should pass a parameter down that says whether checks are necessary.  You can also use that parameter to e.g. suppress checks when constructing local variables and temporaries, although you may already have an alternative mechanism for doing that.
> > Passing parameter that inctructs whether to generate checks or not hardly can be directly implemented. This information is used in `EmitCXXConstructorCall` and it is formed during processing of `new` expression. The distance between these points can be 10 call frames, in some of them (`StmtVisitor` interface of `AggExprEmitter`) we cannot change function parameters.
> > The case of `new` expression in default arguments indeed handled incorrectly, thank you for the catch.  The updated version must process this case correctly.
> I'm not going to accept a patch based on global state here.  `AggValueSlot` already propagates a bunch of flags about the slot into which we're emitting an expression; I don't think it would be difficult to propagate some sort of "alignment is statically known or already checked" flag along with that.
+ 1. @rjmccall offered similar advice in D25448, which led us to find a few more bugs / missed opportunities we otherwise wouldn't have.


Repository:
  rC Clang

https://reviews.llvm.org/D49589