Re: r313784 - Remove offset size check in nullptr arithmetic handling

[Dimitry Andric via cfe-commits]

On 20 Sep 2017, at 20:06, Andrew Kaylor via cfe-commits 
 wrote:
> 
> Author: akaylor
> Date: Wed Sep 20 11:06:44 2017
> New Revision: 313784
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=313784=rev
> Log:
> Remove offset size check in nullptr arithmetic handling
> 
> Differential Revision: https://reviews.llvm.org/D37042

Hi Andrew,

Interestingly, this change (and its related r313666) break PHP 5.6's opcache 
module, as reported in https://bugs.freebsd.org/227723.

PHP 5 uses a construct like to the following, to store multiple items in a 
'variable' struct [1]:

typedef struct TV {
  long tmp;
  char tmp3[30];
} temp_variable;

#define EX_TMP_VAR(ex, n)  ((temp_variable*)(((char*)(ex)) + 
((int)(n
#define EX_TMP_VAR_NUM(ex, n)  (EX_TMP_VAR(ex, 0) - (1 + (n)))
#define VAR_NUM(v) ((unsigned int)(EX_TMP_VAR_NUM(0, 0) - 
EX_TMP_VAR(0, v)))

Invoking the VAR_NUM() macro gives different answers before and after r313784, 
e.g. at r313783:

VAR_NUM(-100) -> 1

At r313784:

   VAR_NUM(-100) -> 0xf99c

I proposed a workaround which avoids the null pointer arithmetic, but can you 
shed any light as to why your changes seem to make matters worse for PHP, while 
they seem to have been intended to make this kind of construct work better 
instead?

-Dimitry

[1] See also: 
https://github.com/php/php-src/blob/PHP-5.6/Zend/zend_compile.h#L417
 and: 
https://github.com/php/php-src/blob/PHP-5.6/ext/opcache/Optimizer/zend_optimizer_internal.h#L28



signature.asc
Description: Message signed with OpenPGP
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r313784 - Remove offset size check in nullptr arithmetic handling

[Andrew Kaylor via cfe-commits]

Author: akaylor
Date: Wed Sep 20 11:06:44 2017
New Revision: 313784

URL: http://llvm.org/viewvc/llvm-project?rev=313784=rev
Log:
Remove offset size check in nullptr arithmetic handling

Differential Revision: https://reviews.llvm.org/D37042


Modified:
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/test/CodeGen/nullptr-arithmetic.c
cfe/trunk/test/Sema/pointer-addition.c
cfe/trunk/test/SemaCXX/nullptr-arithmetic.cpp

Modified: cfe/trunk/lib/AST/Expr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=313784=313783=313784=diff
==
--- cfe/trunk/lib/AST/Expr.cpp (original)
+++ cfe/trunk/lib/AST/Expr.cpp Wed Sep 20 11:06:44 2017
@@ -1837,17 +1837,14 @@ bool BinaryOperator::isNullPointerArithm
 
   // Check that we have one pointer and one integer operand.
   Expr *PExp;
-  Expr *IExp;
   if (LHS->getType()->isPointerType()) {
 if (!RHS->getType()->isIntegerType())
   return false;
 PExp = LHS;
-IExp = RHS;
   } else if (RHS->getType()->isPointerType()) {
 if (!LHS->getType()->isIntegerType())
   return false;
 PExp = RHS;
-IExp = LHS;
   } else {
 return false;
   }
@@ -1862,10 +1859,6 @@ bool BinaryOperator::isNullPointerArithm
   if (!PTy || !PTy->getPointeeType()->isCharType())
 return false;
 
-  // Check that the integer type is pointer-sized.
-  if (Ctx.getTypeSize(IExp->getType()) != Ctx.getTypeSize(PExp->getType()))
-return false;
-
   return true;
 }
 InitListExpr::InitListExpr(const ASTContext , SourceLocation lbraceloc,

Modified: cfe/trunk/test/CodeGen/nullptr-arithmetic.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/nullptr-arithmetic.c?rev=313784=313783=313784=diff
==
--- cfe/trunk/test/CodeGen/nullptr-arithmetic.c (original)
+++ cfe/trunk/test/CodeGen/nullptr-arithmetic.c Wed Sep 20 11:06:44 2017
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -S %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -S %s -emit-llvm -triple i686-unknown-unknown -o - | 
FileCheck %s
+// RUN: %clang_cc1 -S %s -emit-llvm -triple x86_64-unknown-unknown -o - | 
FileCheck %s
 
 #include 
 
@@ -32,3 +34,14 @@ int8_t* test3(intptr_t n) {
 // CHECK-LABEL: test3
 // CHECK: getelementptr
 // CHECK-NOT: inttoptr
+
+// This checks the case where the offset isn't pointer-sized.
+// The front end will implicitly cast the offset to an integer, so we need to
+// make sure that doesn't cause problems on targets where integers and pointers
+// are not the same size.
+int8_t *test4(int8_t b) {
+  return NULLPTRI8 + b;
+}
+// CHECK-LABEL: test4
+// CHECK: inttoptr
+// CHECK-NOT: getelementptr

Modified: cfe/trunk/test/Sema/pointer-addition.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/pointer-addition.c?rev=313784=313783=313784=diff
==
--- cfe/trunk/test/Sema/pointer-addition.c (original)
+++ cfe/trunk/test/Sema/pointer-addition.c Wed Sep 20 11:06:44 2017
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11
+// RUN: %clang_cc1 %s -fsyntax-only -triple i686-unknown-unknown -verify 
-pedantic -Wextra -std=c11
+// RUN: %clang_cc1 %s -fsyntax-only -triple x86_64-unknown-unknown -verify 
-pedantic -Wextra -std=c11
 
 #include 
 

Modified: cfe/trunk/test/SemaCXX/nullptr-arithmetic.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/nullptr-arithmetic.cpp?rev=313784=313783=313784=diff
==
--- cfe/trunk/test/SemaCXX/nullptr-arithmetic.cpp (original)
+++ cfe/trunk/test/SemaCXX/nullptr-arithmetic.cpp Wed Sep 20 11:06:44 2017
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11
+// RUN: %clang_cc1 %s -fsyntax-only -triple i686-unknown-unknown -verify 
-pedantic -Wextra -std=c++11
+// RUN: %clang_cc1 %s -fsyntax-only -triple x86_64-unknown-unknown -verify 
-pedantic -Wextra -std=c++11
 
 #include 
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits