[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 <https://reviews.llvm.org/D58821#1416212>, @joerg wrote:

> Can you include a patch for something like (int *)0xdeadbeeeeeef 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