[Lldb-commits] [PATCH] D70574: [lldb-server] Verify 'g' is supported before relying on them

guiandrade created this revision. guiandrade added reviewers: jasonmolenda, labath. guiandrade added a project: LLDB. Herald added a subscriber: lldb-commits. This ensures that 'g' packets are only used if the config plugin.process.gdb-remote.use-g-packet-for-reading = true and we get a normal pac

[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

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

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

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] D68874: [lldb] Fix offset intersection bug between MPX and AVX registers

guiandrade created this revision. guiandrade added a reviewer: labath. guiandrade added a project: LLDB. Herald added subscribers: lldb-commits, JDevlieghere. This change increases the offset of MPX registers (by 128) so they do not overlap with the offset associated with AVX registers. That was c

[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

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

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] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added a comment. In D67022#1665800 , @labath wrote: > looks fine to me Thank you, guys! Could you please submit it for me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67022/new/ https://revie

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added a comment. Maybe we could use the previous workaround until we find something better? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67022/new/ https://reviews.llvm.org/D67022 ___ lldb-

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade updated this revision to Diff 219372. guiandrade edited the summary of this revision. guiandrade added a comment. Herald added subscribers: kadircet, ilya-biryukov. Falling back to the previous workaround to test EnsureAllDIEsInDeclContextHaveBeenParsed. Repository: rG LLVM Github

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added a comment. In D67022#1660979 , @clayborg wrote: > In D67022#1660945 , @guiandrade > wrote: > > > In D67022#1660901 , @clayborg > > wrote: > > > > > This lo

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added a comment. In D67022#1660901 , @clayborg wrote: > This looks like a good approach to me. Pavel? Even without tests? Just to clarify my last comment, I could not find a way to use the command "target modules dump as" to ensure we won't h

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added a comment. In D67022#1659155 , @labath wrote: > Thanks. The code looks fine to me. Losing the ability to unit test is a bit > of a pity, but since test was pretty icky to begin with, I can't say I'm > going to miss it too much... > One

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added inline comments. Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:27 // defining here, causing this test to fail, feel free to delete it. -TEST(DWARFASTParserClangTests, - TestGetDIEForDeclContextReturnsOnlyMatchingEntries) { - Clang

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade updated this revision to Diff 218779. guiandrade added a comment. Migrating to EnsureAllDIEsInDeclContextHaveBeenParsed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67022/new/ https://reviews.llvm.org/D67022 Files: lldb/source/Plugin

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added a comment. In D67022#1658057 , @clayborg wrote: > In D67022#1656874 , @labath wrote: > > > Thanks for the clarification Greg. > > > > Functionally, this patch seems fine, but I am still wondering if

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade added inline comments. Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:48 ASSERT_EQ(2u, die_list.size()); - ASSERT_EQ(die2, die_list[0]); - ASSERT_EQ(die3, die_list[1]); + ASSERT_NE(die_list.end(), std::find(die_list.begin(), die_list.end

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

guiandrade updated this revision to Diff 218581. guiandrade retitled this revision from "Cache expanded CompilerDeclContext's to avoid parsing them multiple times" to "Enhance SymbolFileDWARF::ParseDeclsForContext performance". guiandrade added a comment. Renaming the test Repository: rG LLV

[Lldb-commits] [PATCH] D67022: Cache expanded CompilerDeclContext's to avoid parsing them multiple times

guiandrade added a comment. Thank you so much for the thorough explanation and ideas, @labath and @clayborg. I forgot to mention that the initial motivation behind this change was to reduce those find calls as I saw they add up to a lot (10^5 calls debugging a Unreal engine sample at certain br

[Lldb-commits] [PATCH] D67022: Cache expanded CompilerDeclContext's to avoid parsing them multiple times

guiandrade updated this revision to Diff 218564. guiandrade marked 6 inline comments as done. guiandrade retitled this revision from "Skip getting declarations for repeated DIEs (WIP)" to "Cache expanded CompilerDeclContext's to avoid parsing them multiple times". guiandrade edited the summary of

[Lldb-commits] [PATCH] D67022: Skip getting declarations for repeated DIEs (WIP)

guiandrade added a comment. Hey guys, This change is more for me to get to know what you guys think. I've noticed that GetDeclForUIDFromDWARF() calls inside SymbolFileDWARF::ParseDeclsForContext (https://github.com/llvm/llvm-project/blob/ef82098a800178a1f973abb8af86eaa690a29734/lldb/source/Plu

[Lldb-commits] [PATCH] D67022: Skip getting declarations for repeated DIEs (WIP)

guiandrade created this revision. guiandrade added reviewers: clayborg, labath. guiandrade added a project: LLDB. Herald added subscribers: lldb-commits, JDevlieghere, aprantl. guiandrade added a comment. Hey guys, This change is more for me to get to know what you guys think. I've noticed that

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

guiandrade added a comment. Sounds good, thank you! Can someone please merge this for me? ninja check-lldb Testing Time: 163.56s Expected Passes: 1603 Expected Failures : 4 Unsupported Tests : 85 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

guiandrade updated this revision to Diff 216166. guiandrade marked 3 inline comments as done. guiandrade added a comment. I'm fixing the style issues pointed by @labath, but I acknowledge the flakiness of this test the way it is right now and am open to deleting it. I can also try to spend some

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

guiandrade updated this revision to Diff 215966. guiandrade added a comment. Herald added a subscriber: mgorny. I tried to write a unit test following @labath's suggestion of creating a test class that inherits from `DWARFASTParserClang`. Please let me know what you guys think. Thanks! Reposi

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

guiandrade added a comment. In D66357#1633461 , @clayborg wrote: > Needs a test, but looks good. Sure, I agree. Though, it's not total clear to me how we could do that. I imagined something like the following. TEST_F(DWARFASTParserClangTests,

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

guiandrade created this revision. guiandrade added reviewers: clayborg, labath. guiandrade added projects: LLDB, clang. Herald added subscribers: lldb-commits, teemperor. guiandrade added a comment. This is a follow up of the investigation I mentioned in http://lists.llvm.org/pipermail/lldb-dev/2

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

guiandrade added a comment. This is a follow up of the investigation I mentioned in http://lists.llvm.org/pipermail/lldb-dev/2019-August/015324.html. Please let me know if you guys think this makes sense. Thanks! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66

[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

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

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

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

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

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

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

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

[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

guiandrade added a comment. In D62221#1520586 , @labath wrote: > BTW, do you have commit access, or you need someone to commit this for you? I do not, I would need someone to help me commit that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

guiandrade updated this revision to Diff 201790. guiandrade added a comment. Remove redundant cast from GDBRemoteCommunicationServerLLGS::Handle_g and add annotation to TestGdbRemoteGPacket::g_returns_correct_data so it gets skipped if the running architecture isn't x86_64 Repository: rG LL

[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

guiandrade updated this revision to Diff 201583. guiandrade added a comment. Add inferior to to set the values of a few registers to known values so we can verify the 'g' packet looks good. Also, start testing with/without thread suffix . Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

guiandrade added a comment. Thank you so much for the feedback! In D62221#1511035 , @clayborg wrote: > Using 'g' packets seems fine to me and we might be able to just enable it. We > will need to ensure that this doesn't regress stepping times so we will

[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

guiandrade updated this revision to Diff 200846. guiandrade marked an inline comment as done. guiandrade edited the summary of this revision. guiandrade added a comment. Making this revision be responsible only for adding the 'g' packet handler and tests. Also, I'm starting to implement a stronge

[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

guiandrade created this revision. guiandrade added a reviewer: labath. guiandrade added a project: LLDB. Herald added a subscriber: lldb-commits. This change makes it possible to fetch all registers at once from Linux by defining a macro. This is useful for the case when all registers are being f