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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
