[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355491: Inline asm constraints: allow ICE-like pointers for 
the n constraint (PR40890) (authored by hans, committed by ).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D58821?vs=189319=189463#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821

Files:
  include/clang/AST/APValue.h
  lib/AST/APValue.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGStmt.cpp
  lib/Sema/SemaStmtAsm.cpp
  test/CodeGen/x86-64-inline-asm.c
  test/Sema/inline-asm-validate-x86.c

Index: include/clang/AST/APValue.h
===
--- include/clang/AST/APValue.h
+++ include/clang/AST/APValue.h
@@ -262,6 +262,12 @@
 return const_cast(this)->getInt();
   }
 
+  /// Try to convert this value to an integral constant. This works if it's an
+  /// integer, null pointer, or offset from a null pointer. Returns true on
+  /// success.
+  bool toIntegralConstant(APSInt , QualType SrcTy,
+  const ASTContext ) const;
+
   APFloat () {
 assert(isFloat() && "Invalid accessor");
 return *(APFloat*)(char*)Data.buffer;
Index: test/CodeGen/x86-64-inline-asm.c
===
--- test/CodeGen/x86-64-inline-asm.c
+++ test/CodeGen/x86-64-inline-asm.c
@@ -1,6 +1,7 @@
 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -DWARN -verify
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -Werror -verify
+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s -S -o - | FileCheck %s
 void f() {
   asm("movaps %xmm3, (%esi, 2)");
 // expected-note@1 {{instantiated into assembly here}}
@@ -15,3 +16,17 @@
 void g(void) { asm volatile("movd %%xmm0, %0"
 :
 : "m"(var)); }
+
+void pr40890(void) {
+  struct s {
+int a, b;
+  } s;
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeef));
+
+// CHECK-LABEL: pr40890
+// CHECK: #define S_A abcd$0
+// CHECK: #define S_B abcd$4
+// CHECK: #define BEEF abcd$244837814038255
+}
Index: test/Sema/inline-asm-validate-x86.c
===
--- test/Sema/inline-asm-validate-x86.c
+++ test/Sema/inline-asm-validate-x86.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i686 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify -DAMD64 %s
 
 void I(int i, int j) {
   static const int BelowMin = -1;
@@ -137,3 +137,21 @@
   : "0"(i), "O"(64)); // expected-no-error
 }
 
+void pr40890(void) {
+  struct s {
+int a, b;
+  };
+  static struct s s;
+  // This null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  // This offset-from-null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  // This pointer cannot be used as an integer constant expression.
+  __asm__ __volatile__("\n#define GLOBAL_A abcd%0\n" : : "n"()); // expected-error{{constraint 'n' expects an integer constant expression}}
+  // Floating-point is also not okay.
+  __asm__ __volatile__("\n#define PI abcd%0\n" : : "n"(3.14f)); // expected-error{{constraint 'n' expects an integer constant expression}}
+#ifdef AMD64
+  // This arbitrary pointer is fine.
+  __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeef));
+#endif
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -9872,13 +9872,12 @@
   return true;
 }
 
-uint64_t V;
-if (LV.isNullPointer())
-  V = Info.Ctx.getTargetNullPointerValue(SrcType);
-else
-  V = LV.getLValueOffset().getQuantity();
+APSInt AsInt;
+APValue V;
+LV.moveInto(V);
+if (!V.toIntegralConstant(AsInt, SrcType, Info.Ctx))
+  llvm_unreachable("Can't cast this!");
 
-APSInt AsInt = Info.Ctx.MakeIntValue(V, SrcType);
 return Success(HandleIntToIntCast(Info, E, DestType, SrcType, AsInt), E);
   }
 
Index: lib/AST/APValue.cpp
===
--- lib/AST/APValue.cpp
+++ lib/AST/APValue.cpp
@@ -614,6 +614,26 @@
   return Result;
 }
 
+bool APValue::toIntegralConstant(APSInt , QualType SrcTy,
+ const ASTContext ) const {
+  if (isInt()) {
+Result = getInt();
+return true;
+  }
+
+  if (isLValue() && isNullPointer()) {
+Result = 

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

hans updated this revision to Diff 189319.
hans added a comment.

Extract code to a new method in APValue.

Eli, what do you think about something like this? Suggestions for better name 
welcome :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821

Files:
  clang/include/clang/AST/APValue.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/x86-64-inline-asm.c
  clang/test/Sema/inline-asm-validate-x86.c

Index: clang/test/Sema/inline-asm-validate-x86.c
===
--- clang/test/Sema/inline-asm-validate-x86.c
+++ clang/test/Sema/inline-asm-validate-x86.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i686 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify -DAMD64 %s
 
 void I(int i, int j) {
   static const int BelowMin = -1;
@@ -137,3 +137,21 @@
   : "0"(i), "O"(64)); // expected-no-error
 }
 
+void pr40890(void) {
+  struct s {
+int a, b;
+  };
+  static struct s s;
+  // This null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  // This offset-from-null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  // This pointer cannot be used as an integer constant expression.
+  __asm__ __volatile__("\n#define GLOBAL_A abcd%0\n" : : "n"()); // expected-error{{constraint 'n' expects an integer constant expression}}
+  // Floating-point is also not okay.
+  __asm__ __volatile__("\n#define PI abcd%0\n" : : "n"(3.14f)); // expected-error{{constraint 'n' expects an integer constant expression}}
+#ifdef AMD64
+  // This arbitrary pointer is fine.
+  __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeef));
+#endif
+}
Index: clang/test/CodeGen/x86-64-inline-asm.c
===
--- clang/test/CodeGen/x86-64-inline-asm.c
+++ clang/test/CodeGen/x86-64-inline-asm.c
@@ -1,6 +1,7 @@
 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -DWARN -verify
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -Werror -verify
+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s -S -o - | FileCheck %s
 void f() {
   asm("movaps %xmm3, (%esi, 2)");
 // expected-note@1 {{instantiated into assembly here}}
@@ -15,3 +16,17 @@
 void g(void) { asm volatile("movd %%xmm0, %0"
 :
 : "m"(var)); }
+
+void pr40890(void) {
+  struct s {
+int a, b;
+  } s;
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeef));
+
+// CHECK-LABEL: pr40890
+// CHECK: #define S_A abcd$0
+// CHECK: #define S_B abcd$4
+// CHECK: #define BEEF abcd$244837814038255
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -385,11 +385,20 @@
   return StmtError(
   Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
   << Info.getConstraintStr() << InputExpr->getSourceRange());
-llvm::APSInt Result = EVResult.Val.getInt();
-if (!Info.isValidAsmImmediate(Result))
+
+// For compatibility with GCC, we also allow pointers that would be
+// integral constant expressions if they were cast to int.
+llvm::APSInt IntResult;
+if (!EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
+ Context))
+  return StmtError(
+  Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
+  << Info.getConstraintStr() << InputExpr->getSourceRange());
+
+if (!Info.isValidAsmImmediate(IntResult))
   return StmtError(Diag(InputExpr->getBeginLoc(),
 diag::err_invalid_asm_value_for_constraint)
-   << Result.toString(10) << Info.getConstraintStr()
+   << IntResult.toString(10) << Info.getConstraintStr()
<< InputExpr->getSourceRange());
   }
 
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -1838,8 +1838,15 @@
   // (immediate or symbolic), try to emit it as such.
   if (!Info.allowsRegister() && !Info.allowsMemory()) {
 if (Info.requiresImmediateConstant()) {
-  llvm::APSInt AsmConst = 

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

hans marked 2 inline comments as done.
hans added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:1852
+IntResult =
+llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+  else

efriedma wrote:
> hans wrote:
> > efriedma wrote:
> > > This always returns an APSInt with width 64; is that really right?  I 
> > > guess it might not really matter given that it's only going to be used as 
> > > an immediate constant anyway, but it seems weird.
> > I agree it seems a little strange, but I think in practice it's correct. 
> > EVResult.Val.getLValueOffset().getQuantity() returns an int64_t, so we're 
> > not losing any data.
> > 
> > The code that I lifted this from, is using the bitwidth of the casted-to 
> > integer type for the result. But it's still only got maximum 64 bits since 
> > the source, getLValueOffset().getQuantity(), is the same.
> The concern isn't that we would lose data.   I'm more concerned the backend 
> might not be prepared for a value of the "wrong" width.
Oh, I see. I'll change the code to use ASTContext::MakeIntValue with the source 
type. Apparently this works even if the source type is a pointer type; I guess 
that yields an integer of the same width. I think maybe that's the best we can 
do?



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:399
+  IntResult =
+  llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+else

efriedma wrote:
> hans wrote:
> > efriedma wrote:
> > > I think it makes sense to add a method to APValue specifically to do the 
> > > conversion from LValue to an APSInt, whether or not isNullPointer() is 
> > > true, and use it both here and in IntExprEvaluator::VisitCastExpr in 
> > > lib/AST/ExprConstant.cpp.  The logic is sort of subtle (and I'm not 
> > > completely sure it's right for targets where null is not zero, but you 
> > > shouldn't try to fix that here).
> > I agree (and this was also Bill's suggestion above) that it would be nice 
> > to have a utility method for this.
> > 
> > I'm not sure adding one to APValue would work for 
> > IntExprEvaluator::VisitCastExpr though, since that code is actually using 
> > its own LValue class, not an APValue until it's time to return a result.
> > 
> > I frankly also doesn't fully understand what that code is doing. If the 
> > LValue has a base value, it seems to just take that as result and ignore 
> > any offset? This is unknown territory to me, but the way I read it, if 
> > there's an lvalue base, the expression isn't going to come out as an 
> > integer constant. I think.
> > 
> > About null pointers, I'm calling getTargetNullPointerValue() so I think 
> > that should be okay, no?
> Oh, I didn't realize IntExprEvaluator::VisitCastExpr wasn't using the same 
> class to represent the value; that makes it harder to usefully refactor.  But 
> still, it would be good to reduce the duplicated code between here and 
> CodeGen.
> 
> > If the LValue has a base value, it seems to just take that as result and 
> > ignore any offset?
> 
> If there's a base value, it returns the whole LValue, including the base and 
> offset.
> 
> > I'm calling getTargetNullPointerValue() so I think that should be okay
> 
> The issue would be the case where you have a null pointer with an offset, 
> like the case in the bug.  It's sort of inconsistent if null==-1, but 
> null+1==1.  But it's not something we handle consistently elsewhere, anyway, 
> so I guess we can ignore it for now.
Ah, the null pointer issue is interesting, but like you say we don't seem to 
handle this in the cast code I was inspired by here either.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Well, that was a sample to illustrate the point. A full working (and now 
failing) example is:

  static inline void outl(unsigned port, unsigned data) {
if (__builtin_constant_p(port) && port < 0x100) {
  __asm volatile("outl %0,%w1" : : "a"(data), "n"(port));
} else {
  __asm volatile("outl %0,%w1" : : "a"(data), "d"(port));
}
  }
  
  void f(unsigned port) { outl(1, 1); }


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> This fails with the new asm constraint checks, since the dead branch is never 
> pruned.

As far as I know, we didn't touch "i", only "n".  Is there a bug filed for the 
issue you're describing?




Comment at: clang/lib/CodeGen/CGStmt.cpp:1852
+IntResult =
+llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+  else

hans wrote:
> efriedma wrote:
> > This always returns an APSInt with width 64; is that really right?  I guess 
> > it might not really matter given that it's only going to be used as an 
> > immediate constant anyway, but it seems weird.
> I agree it seems a little strange, but I think in practice it's correct. 
> EVResult.Val.getLValueOffset().getQuantity() returns an int64_t, so we're not 
> losing any data.
> 
> The code that I lifted this from, is using the bitwidth of the casted-to 
> integer type for the result. But it's still only got maximum 64 bits since 
> the source, getLValueOffset().getQuantity(), is the same.
The concern isn't that we would lose data.   I'm more concerned the backend 
might not be prepared for a value of the "wrong" width.



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:399
+  IntResult =
+  llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+else

hans wrote:
> efriedma wrote:
> > I think it makes sense to add a method to APValue specifically to do the 
> > conversion from LValue to an APSInt, whether or not isNullPointer() is 
> > true, and use it both here and in IntExprEvaluator::VisitCastExpr in 
> > lib/AST/ExprConstant.cpp.  The logic is sort of subtle (and I'm not 
> > completely sure it's right for targets where null is not zero, but you 
> > shouldn't try to fix that here).
> I agree (and this was also Bill's suggestion above) that it would be nice to 
> have a utility method for this.
> 
> I'm not sure adding one to APValue would work for 
> IntExprEvaluator::VisitCastExpr though, since that code is actually using its 
> own LValue class, not an APValue until it's time to return a result.
> 
> I frankly also doesn't fully understand what that code is doing. If the 
> LValue has a base value, it seems to just take that as result and ignore any 
> offset? This is unknown territory to me, but the way I read it, if there's an 
> lvalue base, the expression isn't going to come out as an integer constant. I 
> think.
> 
> About null pointers, I'm calling getTargetNullPointerValue() so I think that 
> should be okay, no?
Oh, I didn't realize IntExprEvaluator::VisitCastExpr wasn't using the same 
class to represent the value; that makes it harder to usefully refactor.  But 
still, it would be good to reduce the duplicated code between here and CodeGen.

> If the LValue has a base value, it seems to just take that as result and 
> ignore any offset?

If there's a base value, it returns the whole LValue, including the base and 
offset.

> I'm calling getTargetNullPointerValue() so I think that should be okay

The issue would be the case where you have a null pointer with an offset, like 
the case in the bug.  It's sort of inconsistent if null==-1, but null+1==1.  
But it's not something we handle consistently elsewhere, anyway, so I guess we 
can ignore it for now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

The other problem is that we don't use the CFG machinery to prune dead 
branches. Consider the x86 in/out instructions: one variant takes an immediate, 
the other a register. The classic way to deal with that is something like

  static inline void outl(unsigned port, uint32_t value)
  {
if (__builtin_constant_p(port) && port < 0x100) {
  __asm volatile("outl %0,%w1" : : "a" (data), "id" (port));
} else {
 __asm volatile("outl %0,%w1" : : "a" (data), "d" (port));
}
  }

This fails with the new asm constraint checks, since the dead branch is never 
pruned. For other architectures it makes an even greater difference. The main 
reason it is a show stopper: there is no sane workaround that doesn't regress 
code quality.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D58821#1416212 , @joerg wrote:

> Can you include a patch for something like (int *)0xdeadbeef on amd64? 
> That's a valid value for "n", but clearly too large for int. Thanks for 
> looking at this, it is one of the two large remaining show stoppers for the 
> asm constraint check.


What's the other show stopper? Is that also something that regressed from the 
previous release since the "n" constraint got stricter?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

hans updated this revision to Diff 189129.
hans marked 2 inline comments as done.
hans added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/x86-64-inline-asm.c
  clang/test/Sema/inline-asm-validate-x86.c

Index: clang/test/Sema/inline-asm-validate-x86.c
===
--- clang/test/Sema/inline-asm-validate-x86.c
+++ clang/test/Sema/inline-asm-validate-x86.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i686 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify -DAMD64 %s
 
 void I(int i, int j) {
   static const int BelowMin = -1;
@@ -137,3 +137,21 @@
   : "0"(i), "O"(64)); // expected-no-error
 }
 
+void pr40890(void) {
+  struct s {
+int a, b;
+  };
+  static struct s s;
+  // This null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  // This offset-from-null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  // This pointer cannot be used as an integer constant expression.
+  __asm__ __volatile__("\n#define GLOBAL_A abcd%0\n" : : "n"()); // expected-error{{constraint 'n' expects an integer constant expression}}
+  // Floating-point is also not okay.
+  __asm__ __volatile__("\n#define PI abcd%0\n" : : "n"(3.14f)); // expected-error{{constraint 'n' expects an integer constant expression}}
+#ifdef AMD64
+  // This arbitrary pointer is fine.
+  __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeef));
+#endif
+}
Index: clang/test/CodeGen/x86-64-inline-asm.c
===
--- clang/test/CodeGen/x86-64-inline-asm.c
+++ clang/test/CodeGen/x86-64-inline-asm.c
@@ -1,6 +1,7 @@
 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -DWARN -verify
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -Werror -verify
+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s -S -o - | FileCheck %s
 void f() {
   asm("movaps %xmm3, (%esi, 2)");
 // expected-note@1 {{instantiated into assembly here}}
@@ -15,3 +16,17 @@
 void g(void) { asm volatile("movd %%xmm0, %0"
 :
 : "m"(var)); }
+
+void pr40890(void) {
+  struct s {
+int a, b;
+  } s;
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeef));
+
+// CHECK-LABEL: pr40890
+// CHECK: #define S_A abcd$0
+// CHECK: #define S_B abcd$4
+// CHECK: #define BEEF abcd$244837814038255
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -385,11 +385,27 @@
   return StmtError(
   Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
   << Info.getConstraintStr() << InputExpr->getSourceRange());
-llvm::APSInt Result = EVResult.Val.getInt();
-if (!Info.isValidAsmImmediate(Result))
+
+// For compatibility with GCC, we also allow pointers that would be
+// integral constant expressions if they were cast to int.
+llvm::APSInt IntResult;
+if (EVResult.Val.isInt())
+  IntResult = EVResult.Val.getInt();
+else if (EVResult.Val.isLValue() && EVResult.Val.isNullPointer())
+  IntResult = llvm::APSInt::get(
+  Context.getTargetNullPointerValue(InputExpr->getType()));
+else if (EVResult.Val.isLValue() && !EVResult.Val.getLValueBase())
+  IntResult =
+  llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+else
+  return StmtError(
+  Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
+  << Info.getConstraintStr() << InputExpr->getSourceRange());
+
+if (!Info.isValidAsmImmediate(IntResult))
   return StmtError(Diag(InputExpr->getBeginLoc(),
 diag::err_invalid_asm_value_for_constraint)
-   << Result.toString(10) << Info.getConstraintStr()
+   << IntResult.toString(10) << Info.getConstraintStr()
<< InputExpr->getSourceRange());
   }
 
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -1838,8 +1838,22 @@
   // (immediate or symbolic), try to emit it as such.
   if 

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

hans marked 6 inline comments as done.
hans added a comment.

In D58821#1416212 , @joerg wrote:

> Can you include a patch for something like (int *)0xdeadbeef on amd64? 
> That's a valid value for "n", but clearly too large for int. Thanks for 
> looking at this, it is one of the two large remaining show stoppers for the 
> asm constraint check.


You mean to check that we don't truncate or otherwise choke on it? Sure, I'll 
add it.




Comment at: clang/lib/CodeGen/CGStmt.cpp:1852
+IntResult =
+llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+  else

efriedma wrote:
> This always returns an APSInt with width 64; is that really right?  I guess 
> it might not really matter given that it's only going to be used as an 
> immediate constant anyway, but it seems weird.
I agree it seems a little strange, but I think in practice it's correct. 
EVResult.Val.getLValueOffset().getQuantity() returns an int64_t, so we're not 
losing any data.

The code that I lifted this from, is using the bitwidth of the casted-to 
integer type for the result. But it's still only got maximum 64 bits since the 
source, getLValueOffset().getQuantity(), is the same.



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:389
+
+// For compatibility with GCC, we also allows pointers that would be
+// integral constant expressions if they were cast to int.

void wrote:
> s/allows/allow/
Done.



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:394
+  IntResult = EVResult.Val.getInt();
+else if (EVResult.Val.isNullPointer())
+  IntResult = llvm::APSInt::get(

efriedma wrote:
> APValue::isNullPointer() asserts that the value is an LValue; do you need to 
> check for that explicitly here?
Yes I do. Thanks!



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:399
+  IntResult =
+  llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+else

efriedma wrote:
> I think it makes sense to add a method to APValue specifically to do the 
> conversion from LValue to an APSInt, whether or not isNullPointer() is true, 
> and use it both here and in IntExprEvaluator::VisitCastExpr in 
> lib/AST/ExprConstant.cpp.  The logic is sort of subtle (and I'm not 
> completely sure it's right for targets where null is not zero, but you 
> shouldn't try to fix that here).
I agree (and this was also Bill's suggestion above) that it would be nice to 
have a utility method for this.

I'm not sure adding one to APValue would work for 
IntExprEvaluator::VisitCastExpr though, since that code is actually using its 
own LValue class, not an APValue until it's time to return a result.

I frankly also doesn't fully understand what that code is doing. If the LValue 
has a base value, it seems to just take that as result and ignore any offset? 
This is unknown territory to me, but the way I read it, if there's an lvalue 
base, the expression isn't going to come out as an integer constant. I think.

About null pointers, I'm calling getTargetNullPointerValue() so I think that 
should be okay, no?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Can you include a patch for something like (int *)0xdeadbeef on amd64? 
That's a valid value for "n", but clearly too large for int. Thanks for looking 
at this, it is one of the two large remaining show stoppers for the asm 
constraint check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:1852
+IntResult =
+llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+  else

This always returns an APSInt with width 64; is that really right?  I guess it 
might not really matter given that it's only going to be used as an immediate 
constant anyway, but it seems weird.



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:394
+  IntResult = EVResult.Val.getInt();
+else if (EVResult.Val.isNullPointer())
+  IntResult = llvm::APSInt::get(

APValue::isNullPointer() asserts that the value is an LValue; do you need to 
check for that explicitly here?



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:399
+  IntResult =
+  llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+else

I think it makes sense to add a method to APValue specifically to do the 
conversion from LValue to an APSInt, whether or not isNullPointer() is true, 
and use it both here and in IntExprEvaluator::VisitCastExpr in 
lib/AST/ExprConstant.cpp.  The logic is sort of subtle (and I'm not completely 
sure it's right for targets where null is not zero, but you shouldn't try to 
fix that here).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

My opinion doesn't carry as much weight as others who are more familiar with 
the front-end code, but LGTM.

One question, the code you added looks similar. Is there a way to extrapolate 
it into its own function? Maybe yet another `EvaluateAs*` method?




Comment at: clang/lib/Sema/SemaStmtAsm.cpp:389
+
+// For compatibility with GCC, we also allows pointers that would be
+// integral constant expressions if they were cast to int.

s/allows/allow/


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58821/new/

https://reviews.llvm.org/D58821



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

[PATCH] D58821: Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)

[Hans Wennborg via Phabricator via cfe-commits]

hans created this revision.
hans added reviewers: efriedma, rnk, rsmith, void.
Herald added subscribers: jdoerfert, eraman.

Apparently GCC allows this, and there's code relying on it (see bug, which is a 
release blocker for llvm 8).

The idea is to allow expression that would have been allowed if they were cast 
to int. So I based the code on how such a cast would be done (the 
CK_PointerToIntegral case in IntExprEvaluator::VisitCastExpr()).

I'm unfamiliar with this code, especially the LValue variant of APValue, so 
please take a careful look.


https://reviews.llvm.org/D58821

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/x86-64-inline-asm.c
  clang/test/Sema/inline-asm-validate-x86.c

Index: clang/test/Sema/inline-asm-validate-x86.c
===
--- clang/test/Sema/inline-asm-validate-x86.c
+++ clang/test/Sema/inline-asm-validate-x86.c
@@ -137,3 +137,15 @@
   : "0"(i), "O"(64)); // expected-no-error
 }
 
+void pr40890(void) {
+  struct s {
+int a, b;
+  };
+  static struct s s;
+  // This null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  // This offset-from-null pointer can be used as an integer constant expression.
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+  // This pointer cannot be used as an integer constant expression.
+  __asm__ __volatile__("\n#define GLOBAL_A abcd%0\n" : : "n"()); // expected-error{{constraint 'n' expects an integer constant expression}}
+}
Index: clang/test/CodeGen/x86-64-inline-asm.c
===
--- clang/test/CodeGen/x86-64-inline-asm.c
+++ clang/test/CodeGen/x86-64-inline-asm.c
@@ -1,6 +1,7 @@
 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -DWARN -verify
 // RUN: %clang_cc1 -triple x86_64 %s -S -o /dev/null -Werror -verify
+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s -S -o - | FileCheck %s
 void f() {
   asm("movaps %xmm3, (%esi, 2)");
 // expected-note@1 {{instantiated into assembly here}}
@@ -15,3 +16,15 @@
 void g(void) { asm volatile("movd %%xmm0, %0"
 :
 : "m"(var)); }
+
+void pr40890(void) {
+  struct s {
+int a, b;
+  } s;
+  __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
+  __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
+
+// CHECK-LABEL: pr40890
+// CHECK: #define S_A abcd$0
+// CHECK: #define S_B abcd$4
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -385,11 +385,27 @@
   return StmtError(
   Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
   << Info.getConstraintStr() << InputExpr->getSourceRange());
-llvm::APSInt Result = EVResult.Val.getInt();
-if (!Info.isValidAsmImmediate(Result))
+
+// For compatibility with GCC, we also allows pointers that would be
+// integral constant expressions if they were cast to int.
+llvm::APSInt IntResult;
+if (EVResult.Val.isInt())
+  IntResult = EVResult.Val.getInt();
+else if (EVResult.Val.isNullPointer())
+  IntResult = llvm::APSInt::get(
+  Context.getTargetNullPointerValue(InputExpr->getType()));
+else if (EVResult.Val.isLValue() && !EVResult.Val.getLValueBase())
+  IntResult =
+  llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity());
+else
+  return StmtError(
+  Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
+  << Info.getConstraintStr() << InputExpr->getSourceRange());
+
+if (!Info.isValidAsmImmediate(IntResult))
   return StmtError(Diag(InputExpr->getBeginLoc(),
 diag::err_invalid_asm_value_for_constraint)
-   << Result.toString(10) << Info.getConstraintStr()
+   << IntResult.toString(10) << Info.getConstraintStr()
<< InputExpr->getSourceRange());
   }
 
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -1838,8 +1838,22 @@
   // (immediate or symbolic), try to emit it as such.
   if (!Info.allowsRegister() && !Info.allowsMemory()) {
 if (Info.requiresImmediateConstant()) {
-  llvm::APSInt AsmConst = InputExpr->EvaluateKnownConstInt(getContext());
-  return llvm::ConstantInt::get(getLLVMContext(), AsmConst);
+  Expr::EvalResult EVResult;
+  InputExpr->EvaluateAsRValue(EVResult, getContext(), true);
+
+  llvm::APSInt IntResult;
+  if