Re: [lldb-dev] [llvm-dev] [lit] check-all hanging
On Thu, Jan 3, 2019 at 11:30 AM Frédéric Riss wrote: > -llvm-dev + lldb-dev for the lldv test failures. > > On Jan 3, 2019, at 7:33 AM, Joel E. Denny wrote: > > All, > > Thanks for the replies. Kuba: For LLDB, when were things expected to have > improved? It's possible things improved for me at some point, but this > isn't something I've found time to track carefully, and I still see > problems. > > I ran check-all a couple of times last night at r350238, which I pulled > yesterday. Here are the results: > > ``` > > Testing Time: 5043.24s > > Unexpected Passing Tests (2): > lldb-Suite :: functionalities/asan/TestMemoryHistory.py > lldb-Suite :: functionalities/asan/TestReportData.py > > > Failing Tests (54): > Clang :: CXX/modules-ts/basic/basic.link/p2/module.cpp > Clang :: Modules/ExtDebugInfo.cpp > Clang :: Modules/using-directive-redecl.cpp > Clang :: Modules/using-directive.cpp > Clang :: PCH/chain-late-anonymous-namespace.cpp > Clang :: PCH/cxx-namespaces.cpp > Clang :: PCH/namespaces.cpp > LLDB :: ExecControl/StopHook/stop-hook-threads.test > LeakSanitizer-AddressSanitizer-x86_64 :: TestCases/Linux/ > use_tls_dynamic.cc > LeakSanitizer-Standalone-x86_64 :: TestCases/Linux/use_tls_dynamic.cc > MemorySanitizer-X86_64 :: dtls_test.c > MemorySanitizer-lld-X86_64 :: dtls_test.c > lldb-Suite :: > functionalities/register/register_command/TestRegisters.py > lldb-Suite :: tools/lldb-server/TestGdbRemoteRegisterState.py > > > It’s hard to diagnose dotest failures without the log. > (My last reply to this was rejected by the list because I wasn't subscribed. Trying again.) I have no experience debugging lldb. Here's the lit output for the last fail (now at r350377), but let me know if you want something more: ``` FAIL: lldb-Suite :: tools/lldb-server/TestGdbRemoteRegisterState.py (59083 of 59736) TEST 'lldb-Suite :: tools/lldb-server/TestGdbRemoteRegisterState.py' FAILED lldb version 8.0.0 LLDB library dir: /home/jdenny/ornl/llvm-mono-git-build/bin LLDB import library dir: /home/jdenny/ornl/llvm-mono-git-build/bin Libc++ tests will not be run because: Unable to find libc++ installation Skipping following debug info categories: ['dsym', 'gmodules'] Session logs for test failures/errors/unexpected successes will go into directory '/home/jdenny/ornl/llvm-mono-git-build/lldb-test-traces' Command invoked: /home/jdenny/ornl/llvm-mono-git/lldb/test/dotest.py -q --arch=x86_64 -s /home/jdenny/ornl/llvm-mono-git-build/lldb-test-traces --build-dir /home/jdenny/ornl/llvm-mono-git-build/lldb-test-build.noindex -S nm -u CXXFLAGS -u CFLAGS --executable /home/jdenny/ornl/llvm-mono-git-build/./bin/lldb --dsymutil /home/jdenny/ornl/llvm-mono-git-build/./bin/dsymutil --filecheck /home/jdenny/ornl/llvm-mono-git-build/./bin/FileCheck -C /home/jdenny/ornl/llvm-mono-git-build/./bin/clang --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy /home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/tools/lldb-server -p TestGdbRemoteRegisterState.py UNSUPPORTED: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) :: test_grp_register_save_restore_works_no_suffix_debugserver (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) (debugserver tests) FAIL: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) :: test_grp_register_save_restore_works_no_suffix_llgs (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) lldb-server exiting... UNSUPPORTED: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) :: test_grp_register_save_restore_works_with_suffix_debugserver (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) (debugserver tests) FAIL: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) :: test_grp_register_save_restore_works_with_suffix_llgs (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) lldb-server exiting... == FAIL: test_grp_register_save_restore_works_no_suffix_llgs (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) -- Traceback (most recent call last): File "/home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/decorators.py", line 144, in wrapper func(*args, **kwargs) File "/home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteRegisterState.py", line 137, in test_grp_register_save_restore_works_no_suffix_llgs self.grp_register_save_restore_works(USE_THREAD_SUFFIX) File "/home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteRegisterState.py", line 97, in grp_register_save_restore_works context = self.expect_gdbremote_sequence() File "/home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/tools/lldb-s
[lldb-dev] Unreliable process attach on Linux
Consider this example program:
#include
#include
#include
#include
#include
#include
int
main(void)
{
// Target process for the debugger.
pid_t pid = fork();
if (pid < 0)
err(1, "fork");
if (pid == 0)
while (true)
pause();
lldb::SBDebugger::Initialize();
{
auto debugger(lldb::SBDebugger::Create());
if (!debugger.IsValid())
errx(1, "SBDebugger::Create failed");
auto target(debugger.CreateTarget(nullptr));
if (!target.IsValid())
errx(1, "SBDebugger::CreateTarget failed");
lldb::SBAttachInfo attachinfo(pid);
lldb::SBError error;
auto process(target.Attach(attachinfo, error));
if (!process.IsValid())
errx(1, "SBTarget::Attach failed: %s", error.GetCString());
error = process.Detach();
if (error.Fail())
errx(1, "SBProcess::Detach failed: %s", error.GetCString());
}
lldb::SBDebugger::Terminate();
if (kill(pid, SIGKILL) != 0)
err(1, "kill");
if (waitpid(pid, NULL, 0) < 0)
err(1, "waitpid");
return 0;
}
Run it in a loop like this:
$ while ./test-attach ; do date; done
On Linux x86-64 (Fedora 29), with LLDB 7 (lldb-7.0.0-1.fc29.x86_64) and
kernel 4.19.12 (kernel-4.19.12-301.fc29.x86_64), after 100 iterations or
so, attaching to the newly created process fails:
test-attach: SBTarget::Attach failed: lost connection
This also reproduces occasionally with LLDB itself (with “lldb -p PID”).
Any suggestions how to get more information about the cause of this
error?
Thanks,
Florian
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] [llvm-dev] [lit] check-all hanging
> On Jan 4, 2019, at 7:30 AM, Joel E. Denny wrote: > > On Thu, Jan 3, 2019 at 11:30 AM Frédéric Riss wrote: > -llvm-dev + lldb-dev for the lldv test failures. > >> On Jan 3, 2019, at 7:33 AM, Joel E. Denny wrote: >> >> All, >> >> Thanks for the replies. Kuba: For LLDB, when were things expected to have >> improved? It's possible things improved for me at some point, but this >> isn't something I've found time to track carefully, and I still see >> problems. >> >> I ran check-all a couple of times last night at r350238, which I pulled >> yesterday. Here are the results: >> >> ``` >> >> Testing Time: 5043.24s >> >> Unexpected Passing Tests (2): >> lldb-Suite :: functionalities/asan/TestMemoryHistory.py >> lldb-Suite :: functionalities/asan/TestReportData.py >> >> >> Failing Tests (54): >> Clang :: CXX/modules-ts/basic/basic.link/p2/module.cpp >> Clang :: Modules/ExtDebugInfo.cpp >> Clang :: Modules/using-directive-redecl.cpp >> Clang :: Modules/using-directive.cpp >> Clang :: PCH/chain-late-anonymous-namespace.cpp >> Clang :: PCH/cxx-namespaces.cpp >> Clang :: PCH/namespaces.cpp >> LLDB :: ExecControl/StopHook/stop-hook-threads.test >> LeakSanitizer-AddressSanitizer-x86_64 :: >> TestCases/Linux/use_tls_dynamic.cc >> LeakSanitizer-Standalone-x86_64 :: TestCases/Linux/use_tls_dynamic.cc >> MemorySanitizer-X86_64 :: dtls_test.c >> MemorySanitizer-lld-X86_64 :: dtls_test.c >> lldb-Suite :: functionalities/register/register_command/TestRegisters.py >> lldb-Suite :: tools/lldb-server/TestGdbRemoteRegisterState.py > > It’s hard to diagnose dotest failures without the log. > > (My last reply to this was rejected by the list because I wasn't subscribed. > Trying again.) > > I have no experience debugging lldb. Here's the lit output for the last fail > (now at r350377), but let me know if you want something more: > > ``` > FAIL: lldb-Suite :: tools/lldb-server/TestGdbRemoteRegisterState.py (59083 of > 59736) > TEST 'lldb-Suite :: > tools/lldb-server/TestGdbRemoteRegisterState.py' FAILED > lldb version 8.0.0 > LLDB library dir: /home/jdenny/ornl/llvm-mono-git-build/bin > LLDB import library dir: /home/jdenny/ornl/llvm-mono-git-build/bin > Libc++ tests will not be run because: Unable to find libc++ installation > Skipping following debug info categories: ['dsym', 'gmodules'] > > Session logs for test failures/errors/unexpected successes will go into > directory '/home/jdenny/ornl/llvm-mono-git-build/lldb-test-traces' > Command invoked: /home/jdenny/ornl/llvm-mono-git/lldb/test/dotest.py -q > --arch=x86_64 -s /home/jdenny/ornl/llvm-mono-git-build/lldb-test-traces > --build-dir /home/jdenny/ornl/llvm-mono-git-build/lldb-test-build.noindex -S > nm -u CXXFLAGS -u CFLAGS --executable > /home/jdenny/ornl/llvm-mono-git-build/./bin/lldb --dsymutil > /home/jdenny/ornl/llvm-mono-git-build/./bin/dsymutil --filecheck > /home/jdenny/ornl/llvm-mono-git-build/./bin/FileCheck -C > /home/jdenny/ornl/llvm-mono-git-build/./bin/clang --env ARCHIVER=/usr/bin/ar > --env OBJCOPY=/usr/bin/objcopy > /home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/tools/lldb-server > -p TestGdbRemoteRegisterState.py > UNSUPPORTED: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) > :: test_grp_register_save_restore_works_no_suffix_debugserver > (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) (debugserver tests) > FAIL: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) :: > test_grp_register_save_restore_works_no_suffix_llgs > (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) > lldb-server exiting... > UNSUPPORTED: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) > :: test_grp_register_save_restore_works_with_suffix_debugserver > (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) (debugserver tests) > FAIL: LLDB (/home/jdenny/ornl/llvm-mono-git-build/bin/clang-8-x86_64) :: > test_grp_register_save_restore_works_with_suffix_llgs > (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) > lldb-server exiting... > == > FAIL: test_grp_register_save_restore_works_no_suffix_llgs > (TestGdbRemoteRegisterState.TestGdbRemoteRegisterState) > -- > Traceback (most recent call last): > File > "/home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/decorators.py", > line 144, in wrapper > func(*args, **kwargs) > File > "/home/jdenny/ornl/llvm-mono-git/lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteRegisterState.py", > line 137, in test_grp_register_save_restore_works_no_suffix_llgs > self.grp_register_save_restore_works(USE_THREAD_SUFFIX) > File > "/home/jdenny/ornl/llvm-mono-git/lldb/package
[lldb-dev] [Reproducers] SBReproducer RFC
Hi Everyone, In September I sent out an RFC [1] about adding reproducers to LLDB. Over the past few months, I landed the reproducer framework, support for the GDB remote protocol and a bunch of preparatory changes. There's still an open code review [2] for dealing with files, but that one is currently blocked by a change to the VFS in LLVM [3]. The next big piece of work is supporting user commands (e.g. in the driver) and SB API calls. Originally I expected these two things to be separate, but Pavel made a good case [4] that they're actually very similar. I created a prototype of how I envision this to work. As usual, we can differentiate between capture and replay. ## SB API Capture When capturing a reproducer, every SB function/method is instrumented using a macro at function entry. The added code tracks the function identifier (currently we use its name with __PRETTY_FUNCTION__) and its arguments. It also tracks when a function crosses the boundary between internal and external use. For example, when someone (be it the driver, the python binding or the RPC server) call SBFoo, and in its implementation SBFoo calls SBBar, we don't need to record SBBar. When invoking SBFoo during replay, it will itself call SBBar. When a boundary is crossed, the function name and arguments are serialized to a file. This is trivial for basic types. For objects, we maintain a table that maps pointer values to indices and serialize the index. To keep our table consistent, we also need to track return for functions that return an object by value. We have a separate macro that wraps the returned object. The index is sufficient because every object that is passed to a function has crossed the boundary and hence was recorded. During replay (see below) we map the index to an address again which ensures consistency. ## SB API Replay To replay the SB function calls we need a way to invoke the corresponding function from its serialized identifier. For every SB function, there's a counterpart that deserializes its arguments and invokes the function. These functions are added to the map and are called by the replay logic. Replaying is just a matter looping over the function identifiers in the serialized file, dispatching the right deserialization function, until no more data is available. The deserialization function for constructors or functions that return by value contains additional logic for dealing with the aforementioned indices. The resulting objects are added to a table (similar to the one described earlier) that maps indices to pointers. Whenever an object is passed as an argument, the index is used to get the actual object from the table. ## Tool Even when using macros, adding the necessary capturing and replay code is tedious and scales poorly. For the prototype, we did this by hand, but we propose a new clang-based tool to streamline the process. For the capture code, the tool would validate that the macro matches the function signature, suggesting a fixit if the macros are incorrect or missing. Compared to generating the macros altogether, it has the advantage that we don't have "configured" files that are harder to debug (without faking line numbers etc). The deserialization code would be fully generated. As shown in the prototype there are a few different cases, depending on whether we have to account for objects or not. ## Prototype Code I created a differential [5] on Phabricator with the prototype. It contains the necessary methods to re-run the gdb remote (reproducer) lit test. ## Feedback Before moving forward I'd like to get the community's input. What do you think about this approach? Do you have concerns or can we be smarter somewhere? Any feedback would be greatly appreciated! Thanks, Jonas [1] http://lists.llvm.org/pipermail/lldb-dev/2018-September/014184.html [2] https://reviews.llvm.org/D54617 [3] https://reviews.llvm.org/D54277 [4] https://reviews.llvm.org/D55582 [5] https://reviews.llvm.org/D56322 ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
[lldb-dev] Signedness of scalars built from APInt(s)
While adding support for 512-bit integers in `Scalar`, I figured I
could add some coverage.
TEST(ScalarTest, Signedness) {
auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
}
The result of `s1.GetType()` is Scalar::e_sint.
This is because an APInt can't distinguish between "int patatino = 12"
and "uint patatino = 12".
The correct class in `llvm` to do that is `APSInt`.
I think there are at least a couple of possibilities to fix this:
1) Change the constructor in Scalar to always get an APSInt. This
would be fairly invasive but we could always query isSigned() to make
sure we instantiate the correct scalar type.
2) Change the implementation of Scalar(APInt v) to look at the sign
bit, and if that's set, treat the value as signed (and unsigned
otherwise). This will be definitely more correct than the current
implementation which always construct signed types from APInt(s), but
I'm not entirely sure of all the implications.
What do you think?
--
Davide
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Signedness of scalars built from APInt(s)
On Fri, Jan 4, 2019 at 1:57 PM Davide Italiano wrote:
>
> While adding support for 512-bit integers in `Scalar`, I figured I
> could add some coverage.
>
> TEST(ScalarTest, Signedness) {
> auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
> auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
> ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
> ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
> }
>
> The result of `s1.GetType()` is Scalar::e_sint.
> This is because an APInt can't distinguish between "int patatino = 12"
> and "uint patatino = 12".
> The correct class in `llvm` to do that is `APSInt`.
>
Please note that this is also broken in the case where you have
APInt(32 /* bitWidth */, -323);
because of the way the constructor is implemented.
--
Davide
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Signedness of scalars built from APInt(s)
If I understand the situation correctly I think we should do both. I'd
start by doing (2) to improve the current behavior and add a constructor
for APSInt. We can then audit the call sites and migrate to APSInt where
it's obvious that the type is signed. That should match the semantics of
both classes?
On Fri, Jan 4, 2019 at 2:00 PM Davide Italiano
wrote:
> On Fri, Jan 4, 2019 at 1:57 PM Davide Italiano
> wrote:
> >
> > While adding support for 512-bit integers in `Scalar`, I figured I
> > could add some coverage.
> >
> > TEST(ScalarTest, Signedness) {
> > auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
> > auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
> > ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
> > ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
> > }
> >
> > The result of `s1.GetType()` is Scalar::e_sint.
> > This is because an APInt can't distinguish between "int patatino = 12"
> > and "uint patatino = 12".
> > The correct class in `llvm` to do that is `APSInt`.
> >
>
> Please note that this is also broken in the case where you have
> APInt(32 /* bitWidth */, -323);
> because of the way the constructor is implemented.
>
> --
> Davide
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Signedness of scalars built from APInt(s)
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 0x1000 signed or
unsigned? It's a trick question, because there's not enough information!
If it was written "int x = 0x1000" then it's signed (and negative). If
it was written "unsigned x = 0x1000" 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.
On Fri, Jan 4, 2019 at 2:58 PM Jonas Devlieghere
wrote:
> If I understand the situation correctly I think we should do both. I'd
> start by doing (2) to improve the current behavior and add a constructor
> for APSInt. We can then audit the call sites and migrate to APSInt where
> it's obvious that the type is signed. That should match the semantics of
> both classes?
>
> On Fri, Jan 4, 2019 at 2:00 PM Davide Italiano
> wrote:
>
>> On Fri, Jan 4, 2019 at 1:57 PM Davide Italiano
>> wrote:
>> >
>> > While adding support for 512-bit integers in `Scalar`, I figured I
>> > could add some coverage.
>> >
>> > TEST(ScalarTest, Signedness) {
>> > auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
>> > auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
>> > ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
>> > ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
>> > }
>> >
>> > The result of `s1.GetType()` is Scalar::e_sint.
>> > This is because an APInt can't distinguish between "int patatino = 12"
>> > and "uint patatino = 12".
>> > The correct class in `llvm` to do that is `APSInt`.
>> >
>>
>> Please note that this is also broken in the case where you have
>> APInt(32 /* bitWidth */, -323);
>> because of the way the constructor is implemented.
>>
>> --
>> Davide
>>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Signedness of scalars built from APInt(s)
On Fri, Jan 4, 2019 at 3:13 PM Zachary Turner 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 0x1000 signed or
> unsigned? It's a trick question, because there's not enough information!
> If it was written "int x = 0x1000" then it's signed (and negative). If
> it was written "unsigned x = 0x1000" 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.
> On Fri, Jan 4, 2019 at 2:58 PM Jonas Devlieghere
> wrote:
>
>> If I understand the situation correctly I think we should do both. I'd
>> start by doing (2) to improve the current behavior and add a constructor
>> for APSInt. We can then audit the call sites and migrate to APSInt where
>> it's obvious that the type is signed. That should match the semantics of
>> both classes?
>>
>> On Fri, Jan 4, 2019 at 2:00 PM Davide Italiano
>> wrote:
>>
>>> On Fri, Jan 4, 2019 at 1:57 PM Davide Italiano
>>> wrote:
>>> >
>>> > While adding support for 512-bit integers in `Scalar`, I figured I
>>> > could add some coverage.
>>> >
>>> > TEST(ScalarTest, Signedness) {
>>> > auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
>>> > auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
>>> > ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
>>> > ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
>>> > }
>>> >
>>> > The result of `s1.GetType()` is Scalar::e_sint.
>>> > This is because an APInt can't distinguish between "int patatino = 12"
>>> > and "uint patatino = 12".
>>> > The correct class in `llvm` to do that is `APSInt`.
>>> >
>>>
>>> Please note that this is also broken in the case where you have
>>> APInt(32 /* bitWidth */, -323);
>>> because of the way the constructor is implemented.
>>>
>>> --
>>> Davide
>>>
>>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Signedness of scalars built from APInt(s)
Option 3? Add APSInt as a new member?
Current:
Scalar::Type m_type;
llvm::APInt m_integer;
llvm::APFloat m_float;
bool m_ieee_quad = false;
Option #3:
Scalar::Type m_type;
llvm::APInt m_uint;
llvm::APSInt m_sint;
llvm::APFloat m_float;
bool m_ieee_quad = false;
I don't know enough about APInt and APSInt to know if one or the other can
correctly be used for mixed operations.
> On Jan 4, 2019, at 1:57 PM, Davide Italiano wrote:
>
> While adding support for 512-bit integers in `Scalar`, I figured I
> could add some coverage.
>
> TEST(ScalarTest, Signedness) {
> auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
> auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
> ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
> ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
> }
>
> The result of `s1.GetType()` is Scalar::e_sint.
> This is because an APInt can't distinguish between "int patatino = 12"
> and "uint patatino = 12".
> The correct class in `llvm` to do that is `APSInt`.
>
> I think there are at least a couple of possibilities to fix this:
> 1) Change the constructor in Scalar to always get an APSInt. This
> would be fairly invasive but we could always query isSigned() to make
> sure we instantiate the correct scalar type.
> 2) Change the implementation of Scalar(APInt v) to look at the sign
> bit, and if that's set, treat the value as signed (and unsigned
> otherwise). This will be definitely more correct than the current
> implementation which always construct signed types from APInt(s), but
> I'm not entirely sure of all the implications.
>
> What do you think?
>
> --
> Davide
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Signedness of scalars built from APInt(s)
On Fri, Jan 4, 2019 at 3:23 PM Jonas Devlieghere wrote: > On Fri, Jan 4, 2019 at 3:13 PM Zachary Turner 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 0x1000 signed or >> unsigned? It's a trick question, because there's not enough information! >> If it was written "int x = 0x1000" then it's signed (and negative). If >> it was written "unsigned x = 0x1000" 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
Re: [lldb-dev] Signedness of scalars built from APInt(s)
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 wrote: > On Fri, Jan 4, 2019 at 3:23 PM Jonas Devlieghere > wrote: > >> On Fri, Jan 4, 2019 at 3:13 PM Zachary Turner 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 0x1000 signed or >>> unsigned? It's a trick question, because there's not enough information! >>> If it was written "int x = 0x1000" then it's signed (and negative). If >>> it was written "unsigned x = 0x1000" 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
Re: [lldb-dev] Signedness of scalars built from APInt(s)
On Fri, Jan 4, 2019 at 3:54 PM Zachary Turner wrote: > > 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. > OK, I think there might be subtleties we're missing right now but the best way is just try and see what happens. I'll send patches early next week. -- Davide ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
