[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