[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