Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

On Mon, 11 Nov 2019 17:34:45 +0100, Pavel Labath wrote: > On 09/11/2019 21:41, Jan Kratochvil via lldb-commits wrote: > > Linux Fedora 30 x86_64: > > d162e02cee74a3dbbfb1317fa9749f5e18610282 > > > >File > > "/home/jkratoch/redhat/llvm-monorepo/lldb/packages/Python/lldbsuite/test/functionaliti

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

On 09/11/2019 21:41, Jan Kratochvil via lldb-commits wrote: On Sat, 09 Nov 2019 03:25:51 +0100, Jason Molenda via lldb-commits wrote: I'm switching the default for at least the weekend via 60ab30ebce833c87bd4776f67cd9a82fe162ef9c / https://reviews.llvm.org/rG60ab30ebce83 so the bots aren't fai

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

Thanks!  lldb is trying to resume execution for some reason. I wonder if the T11 as the stop reason is causing problems, I just did it at random. I'll try changing the test to use a T02 stop reason. Baffled as to the difference in behavior, but we'll see if this matters. On 11/10/19 10:32 PM,

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

Hi Jason, On Mon, 11 Nov 2019 02:04:55 +0100, Jason Molenda wrote: > ./lldb-dotest -t -v -p TestNoGPacketSupported.py Linux Fedora 30 x86_64 6ef63638cb8bac243e0e59cec66a19c57b79e351 Thanks, Jan /usr/bin/python /home/jkratoch/redhat/llvm-monorepo/lldb/test/API/dotest.py --arch=x86_64 -s /home/

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

On 11/09/19 12:41 PM, Jan Kratochvil wrote: > > On Sat, 09 Nov 2019 03:25:51 +0100, Jason Molenda via lldb-commits wrote: > > I'm switching the default for at least the weekend via > > 60ab30ebce833c87bd4776f67cd9a82fe162ef9c / > > https://reviews.llvm.org/rG60ab30ebce83 so the bots aren't

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

On Sat, 09 Nov 2019 03:25:51 +0100, Jason Molenda via lldb-commits wrote: > I'm switching the default for at least the weekend via > 60ab30ebce833c87bd4776f67cd9a82fe162ef9c / > https://reviews.llvm.org/rG60ab30ebce83 so the bots aren't failing because of > this, we can all look into this next w

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

I'm switching the default for at least the weekend via 60ab30ebce833c87bd4776f67cd9a82fe162ef9c / https://reviews.llvm.org/rG60ab30ebce83 so the bots aren't failing because of this, we can all look into this next week. I think the best solution is to get lldb to fall back to p/P if g/G are not

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

Hm, a follow-on problem is that there's some bug between debugserver and lldb with the g/G packets which is causing bot failures on macos systems. lldb has never used g/G before (if p/P was available) because debugserver seeds all of the GPR values with the stop packet (? aka Tnn) or with the jT

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

Hey Jason, Sorry, I think I ended up accidentally dropping the verification in the final version ( https://github.com/llvm/llvm-project/blob/b1b70f6761266c3eecaf8bd71529eaf51994207b/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp#L307). I had implemented GDBRemoteCommunicationClient::Ge

Re: [Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

A heads-up - lldb is failing to detect the case where the remote gdb RSP stub does not support the 'g' packet. I found this while doing some bare board debugging; g fails and doesn't fall back to fetching register values individually. I wrote a test TestNoGPacketSupported.py to show this beh

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

This revision was automatically updated to reflect the committed changes. Closed by commit rGb1b70f676126: [lldb-server] Add setting to force 'g' packet use (authored by guiandrade, committed by labath). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. In D62931#1726865 , @labath wrote: > This looks fine to me. Thanks for your patience. Do you still need someone to > commit this for you? Np. Yes, I do. Could you please do it for me? Thanks! Repository: rG LLVM Github M

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This looks fine to me. Thanks for your patience. Do you still need someone to commit this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 226958. guiandrade edited the summary of this revision. guiandrade added a comment. Moving to a single config, use-g-packet-for-reading, that forces the use of 'g' packets for reading, but does not force 'G' for writing. The latter only ends up being used

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. In D62931#1724433 , @guiandrade wrote: > In D62931#1719948 , @labath wrote: > > > I'm sorry, this dropped off my radar. The code looks fine, but it could use > > some more tests. For instanc

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

jasonmolenda added a comment. In D62931#1724433 , @guiandrade wrote: > Yeah, more tests would be useful. They made me notice an issue btw. I was > simply sending a 'g' packet and checking if the server replied back nicely; > however, IIUC with 'G' packet

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade marked an inline comment as done. guiandrade added a comment. In D62931#1719948 , @labath wrote: > I'm sorry, this dropped off my radar. The code looks fine, but it could use > some more tests. For instance, one test when you set the setting va

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 226779. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 Files: lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py lldb/

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 226778. guiandrade added a comment. Adding tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 Files: lldb/packages/Python/lldbsuite/test/commands/register/regi

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. I'm sorry, this dropped off my radar. The code looks fine, but it could use some more tests. For instance, one test when you set the setting value to the non-default , and then check that lldb does _not_ use the `g` packet . Comment at: lldb/source/Pl

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. Hey @labath, Have you had a chance to take a look at the last revision of this? Tks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 _

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 224626. guiandrade added a comment. Properly separating MPX fix from this change (I guess) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 Files: lldb/packages/Py

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 224615. guiandrade edited the summary of this revision. guiandrade added a comment. Moving MPX fix to a parent change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. Yeah, I think that's the best we can do right now. Could you just drop the gdb_remote_offset from the `GetPtraceOffset`. It can be recovered from the `index` argument, and I don't want people to think that they _have to_ implement the ptrace offset computation via the gd

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. In D62931#1699746 , @labath wrote: > In D62931#1697999 , @guiandrade > wrote: > > > Thank you for looking into this, @labath > > > > I'd like to fix that, but I'm not sure if I understan

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 224120. guiandrade added a comment. Dissociating the use of g/G packets and trying to address the AVX/MPX offset bug Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. In D62931#1697999 , @guiandrade wrote: > Thank you for looking into this, @labath > > I'd like to fix that, but I'm not sure if I understand the code well enough/ > it's not clear to me what the solution would look like. > > > I th

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. Thank you for looking into this, @labath I'd like to fix that, but I'm not sure if I understand the code well enough/ it's not clear to me what the solution would look like. > I think the only reasonable way to do that would be to change the > RegisterInfo offset fo

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. Yeah, that is a problem, though it's not really caused by you, and I am not sure even if your planned changed makes it worse (it looks like it *might* do that, but I'd have to dig deeper to tell for sure). If you found this issue by running the test suite, then I guess i

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. In D62931#1607800 , @labath wrote: > In D62931#1607221 , @guiandrade > wrote: > > > Oh, sorry about that. I was relying on `ninja check`, which runs okay for > > me. > > > > > ninja c

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. In D62931#1607221 , @guiandrade wrote: > Oh, sorry about that. I was relying on `ninja check`, which runs okay for me. > > > ninja check >[0/1] Running the LLVM regression tests >Testing Time: 583.96s > Expected Passe

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. Oh, sorry about that. I was relying on `ninja check`, which runs okay for me. > ninja check [0/1] Running the LLVM regression tests Testing Time: 583.96s Expected Passes: 32141 Expected Failures : 147 Unsupported Tests : 438 How can I invoke th

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath requested changes to this revision. labath added a comment. This revision now requires changes to proceed. Hm... this patch spectacularly fails for me with about 100 failures. Are you sure you ran the test suite with this change? I took a brief look at `x86-64-ymm-write.test` as it was ea

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 212200. guiandrade added a comment. Adding support for tablegen generated properties introduced in https://reviews.llvm.org/D65185 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.ll

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. Unfortunately this patch has raced with the latest tablegen changes, and it no longer applies cleanly. Could you please rebase on the current trunk? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.l

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. Thanks, labath@ Could anyone please merge this for me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 ___ lldb-commits mailing lis

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. LGTM. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 __

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 211781. guiandrade marked an inline comment as done. guiandrade added a comment. Adds a test to make sure the client is using 'g' by default. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https:/

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. Sorry for the delay. The patch looks fine to me. As far as testing goes, there's a "gdb-client" test suite in `test/testcases/functionalities/gdb_remote_client/`, which should allow you to mock gdb-server responses and verify that the right packets are being sent. ===

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 209063. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm.org/D62931 Files: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/source/Plugins/Process/

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

clayborg added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.h:103 lldb_private::LazyBool m_associated_with_libdispatch_queue; + bool m_use_g_packet; Not a big deal, but might be easier to store this in ProcessGDBRemote

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:194 +const uint32_t idx = ePropertyUseGPacket; +return m_collection_sp->GetPropertyAtIndexAsBoolean(nullptr, idx, true); + } Should we default to tr

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade updated this revision to Diff 208838. guiandrade marked 3 inline comments as done. guiandrade added a comment. This rebases the change, rename *try_to_use_g_packet* to *use_g_packet*, defaults the setting to true, and modifies the condition of the read_all_registers_at_once bool in T

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added a comment. I think it will be pretty hard for a single person to gather all of these stats for all targets. So perhaps the best solution is to have the setting for the `g` packet, defaulting to true. After a while, if nobody notices a regression, we can just remove it... Reposito

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. Just checking the status of this change. Are we still considering enabling 'g' packets unconditionally, or are we better off using the setting for safety? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62931/new/ https://reviews.llvm

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade added a comment. In D62931#1532510 , @clayborg wrote: > In D62931#1531965 , @labath wrote: > > > > > > There are so many various GDB remote servers and it is hard to say what will > work for most people.

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

clayborg added a comment. In D62931#1531965 , @labath wrote: > Personally, I'm wondering whether we even need a setting for this. Running > the speed test command locally (i.e. the lowest latency we can possibly get), > I get results like: > > (lldb) p

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

labath added reviewers: clayborg, jasonmolenda. labath added a comment. Personally, I'm wondering whether we even need a setting for this. Running the speed test command locally (i.e. the lowest latency we can possibly get), I get results like: (lldb) process plugin packet speed-test --count

[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

guiandrade created this revision. guiandrade added a reviewer: labath. guiandrade added projects: LLDB, LLVM. Herald added a subscriber: lldb-commits. Following up on https://reviews.llvm.org/D62221 , this change introduces the setting plugin.process.gdb-remote.tr