It seems like we have 3 uses of this constructor in LLDB. IRInterpreter.cpp: Constructs a Scalar for an llvm::Constant. IRForTarget.cpp: Constructs a Scalar for an llvm::Constant. ClangASTContext.cpp: bitcasts an APFloat to an APInt.
The first two we should just treat constants in LLVM IR as signed, so we could construct an APSInt at the call-sites with signed=true. The third seems like a bug, we should just have a Scalar constructor that takes an APFloat directly. We already have one that takes a float and it just sets the `APFloat m_float` member variable, I don't know why we're jumping through this bitcast hoop (which is probably wrong for negative floats anyway). On Fri, Jan 4, 2019 at 3:38 PM Zachary Turner <ztur...@google.com> wrote: > On Fri, Jan 4, 2019 at 3:23 PM Jonas Devlieghere <jo...@devlieghere.com> > wrote: > >> On Fri, Jan 4, 2019 at 3:13 PM Zachary Turner <ztur...@google.com> wrote: >> >>> I don't think #2 is a correct change. Just because the sign bit is set >>> doesn't mean it's signed. Is the 4-byte value 0x10000000 signed or >>> unsigned? It's a trick question, because there's not enough information! >>> If it was written "int x = 0x10000000" then it's signed (and negative). If >>> it was written "unsigned x = 0x10000000" then it's unsigned (and >>> positive). What about the 4-byte value 0x1? Still a trick! If it was >>> written "int x = 1" then it's signed (and positive), and if it was written >>> "unsigned x = 1" then it's unsigned (and positive). >>> >>> My point is that signedness of the *type* does not necessarly imply >>> signedness of the value, and vice versa. >>> >>> APInt is purely a bit-representation and a size, there is no information >>> whatsoever about whether type *type* is signed. It doesn't make sense to >>> say "is this APInt negative?" without additional information. >>> >>> With APSInt, on the other hand, it does make sense to ask that >>> question. If you have an APSInt where isSigned() is true, *then* you can >>> use the sign bit to determine whether it's negative. And if you have an >>> APSInt where isSigned() is false, then the "sign bit" is not actually a >>> sign bit at all, it is just an extra power of 2 for the unsigned value. >>> >>> This is my understanding of the classes, someone correct me if I'm wrong. >>> >> >>> IIUC though, the way to fix this is by using APSInt throughout the >>> class, and delete all references to APInt. >>> >> >> I think we share the same understanding. If we know at every call site >> whether the type is signed or not then I totally agree, we should only use >> APSInt. The reason I propose doing (2) first is for the first scenario you >> described, where you don't know. Turning it into an explicit APSInt is as >> bad as using an APInt and looking at the value. The latter has the >> advantage that it conveys that you don't know, while the other may or may >> not be a lie. >> > > Do we ever not know though? And if so, then why don't we know whether the > type is supposed to be signed or unsigned? Because guessing is always > going to be wrong sometimes. >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
