[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

sylvestre.ledru added a comment. For the record, Firefox was using this trick. This patch is breaking a ci build (clang trunk + warning as errors) More information here: https://bugzilla.mozilla.org/show_bug.cgi?id=1402362 Repository: rL LLVM https://reviews.llvm.org/D37042

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added a comment. I agree, it doesn't add much value. Either way, though, please make sure you address the buildbot failures quickly. Repository: rL LLVM https://reviews.llvm.org/D37042 ___ cfe-commits mailing list

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added a comment. This is breaking buildbots for 32-bit targets because the offset in 'nullptr + int8_t_N' is being implicitly cast to an int. That makes the sizeof(offset) == sizeof(ptr) check turn out differently than my tests were assuming. I can get the buildbots green

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

This revision was automatically updated to reflect the committed changes. Closed by commit rL313666: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc (authored by akaylor). Changed prior to commit: https://reviews.llvm.org/D37042?vs=115262=115889#toc Repository: rL LLVM

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D37042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor updated this revision to Diff 115262. andrew.w.kaylor added a comment. -Changed GNU idiom from extension to warning. -Updated to ToT. https://reviews.llvm.org/D37042 Files: include/clang/AST/Expr.h include/clang/Basic/DiagnosticGroups.td

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, andrew.w.kaylor

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, efriedma

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, andrew.w.kaylor

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, efriedma

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, "extension" isn't

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added a comment. Does anything else need to be done for this to be ready to land? https://reviews.llvm.org/D37042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added a comment. Ping. https://reviews.llvm.org/D37042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor updated this revision to Diff 113343. andrew.w.kaylor added a comment. Fixed value-dependent argument in isNullPointerConstant checks. Added check for C++ zero offset in subtraction. Added value-dependent test cases. https://reviews.llvm.org/D37042 Files:

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

rsmith added inline comments. Comment at: lib/AST/Expr.cpp:1857 + if (!PExp->IgnoreParenCasts() + ->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNull)) +return false; andrew.w.kaylor wrote: > rsmith wrote: > > If we get here with a

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added inline comments. Comment at: lib/AST/Expr.cpp:1857 + if (!PExp->IgnoreParenCasts() + ->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNull)) +return false; rsmith wrote: > If we get here with a value-dependent

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; efriedma wrote: > andrew.w.kaylor wrote: > > efriedma wrote: > > >

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

rsmith added inline comments. Comment at: lib/AST/Expr.cpp:1857 + if (!PExp->IgnoreParenCasts() + ->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNull)) +return false; If we get here with a value-dependent expression, we should treat it as

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor updated this revision to Diff 113311. andrew.w.kaylor added a comment. Refactored the GNU idiom check to be shared by Sema and CodeGen. Refined the checks so that nullptr+0 doesn't warn in C++. Added the zero offset qualification in the warning produced with C++.

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; andrew.w.kaylor wrote: > efriedma wrote: > > rsmith wrote: > > >

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; efriedma wrote: > rsmith wrote: > > andrew.w.kaylor wrote: >

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; rsmith wrote: > andrew.w.kaylor wrote: > > efriedma wrote: > > >

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

rsmith added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; andrew.w.kaylor wrote: > efriedma wrote: > > Please make sure you use

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; efriedma wrote: > Please make sure you use exactly the same

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6032 +def ext_gnu_null_ptr_arith : Extension< + "inttoptr casting using arithmetic on a null pointer is a GNU extension">, + InGroup; rsmith wrote: > efriedma wrote:

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

rsmith added a comment. In https://reviews.llvm.org/D37042#855713, @efriedma wrote: > I didn't think of this earlier, but strictly speaking, I think > "(char*)nullptr+0" isn't undefined in C++? Yes, that's correct. (C++'s model is basically equivalent to having an object of size zero at the

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added a reviewer: rsmith. efriedma added a comment. I didn't think of this earlier, but strictly speaking, I think "(char*)nullptr+0" isn't undefined in C++? But probably worth emitting the warning anyway; anyone writing out arithmetic on null is probably doing something suspicious,

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor updated this revision to Diff 113134. andrew.w.kaylor added a comment. Added warnings for null pointer arithmetic. https://reviews.llvm.org/D37042 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/CodeGen/CGExprScalar.cpp

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

hfinkel added a comment. In https://reviews.llvm.org/D37042#852733, @efriedma wrote: > It would be nice to warn on any nullptr arithmetic, yes. (We probably want > the wording of the warning to be a bit different if we're activating this > workaround.) +1 (I'll likely want the ability to

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added a comment. It would be nice to warn on any nullptr arithmetic, yes. (We probably want the wording of the warning to be a bit different if we're activating this workaround.) https://reviews.llvm.org/D37042 ___ cfe-commits mailing

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added a comment. In https://reviews.llvm.org/D37042#850676, @hfinkel wrote: > I'd really like to see this done in some way such that we can issue a warning > along with generating well-defined code. The warning can specifically state > that the code is using an extension, etc.

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

hfinkel added a comment. I'd really like to see this done in some way such that we can issue a warning along with generating well-defined code. The warning can specifically state that the code is using an extension, etc. https://reviews.llvm.org/D37042

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added a comment. My intention here was to make this as strict/limited as possible while still handling the cases of interest. There are two different implementations I want to handle. You can see the first variation in the __BPTR_ALIGN definition being added here:

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

majnemer added a comment. In https://reviews.llvm.org/D37042#849726, @majnemer wrote: > I'd expect this to be more limited. > > Something like, "if we have a BinaryOperator of + between a CStyleCastExpr of > NullToPointer and 'X', emit inttoptr(X)" Although maybe that is too strict... I guess

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

majnemer added a comment. I'd expect this to be more limited. Something like, "if we have a BinaryOperator of + between a CStyleCastExpr of NullToPointer and 'X', emit inttoptr(X)" https://reviews.llvm.org/D37042 ___ cfe-commits mailing list

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor added a comment. I'm not sure why the svn attributes got attached to the file I added. I'll remove them before committing. https://reviews.llvm.org/D37042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

andrew.w.kaylor created this revision. This patch adds a hack to clang's emitPointerArithmetic() function to get it to emit an inttoptr instruction rather than a GEP with a null base pointer when the 'p = nullptr + n' idiom is used to convert a pointer-sized integer to a pointer. This idiom