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
