[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 30 15:20:34 PDT 2017
rjmccall added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:8808
+ Context.getTypeSize(pointerType) ==
+ IsGNUIdiom = true;
> andrew.w.kaylor wrote:
> > efriedma wrote:
> > > rsmith wrote:
> > > > andrew.w.kaylor wrote:
> > > > > efriedma wrote:
> > > > > > Please make sure you use exactly the same check in Sema and CodeGen (probably easiest to stick a helper into lib/AST/).
> > > > > That's a good idea, but I'm not really familiar enough with the structure of clang to be sure I'm not doing it in a ham-fisted way. Can you clarify? Are you suggesting adding something like ASTContext::isPointeeTypeCharSize() and ASTContext::isIntegerTypePointerSize()? Or maybe a single specialized function somewhere that does both checks like ASTContext::areOperandNullPointerArithmeticCompatible()?
> > > > I would suggest something even more specific, such as `isNullPointerPlusIntegerExtension`
> > > I'm most concerned about the difference between "isa<llvm::ConstantPointerNull>(pointer)" and "PExp->IgnoreParenCasts()->isNullPointerConstant()"... they're different in important ways.
> > >
> > > I was thinking something like "BinaryOperator::isNullPointerArithmeticExtension()"
> > At this point in Sema the BinaryOperator for the addition hasn't been created yet, right? So a static function that takes the opcode and operands?
> I wasn't really thinking about that... but yes, something like that.
I've long thought that it would make sense to store a semantic operation kind in BinaryOperator and UnaryOperator instead of treating the operator alone as a sufficient discriminator. Of course the operator is *usually* sufficient, but there are cases where that's not true — for example, we could use that operation kind to distinguish integer and pointer arithmetic, and maybe even to identify which side of a + is the pointer.
If we had that, we could very easily flag a null+int operation as a completely different semantic operation, which it basically is.
More information about the cfe-commits