Page MenuHomePhabricator

guiandrade (Guilherme Andrade)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 16 2019, 12:23 PM (131 w, 3 d)

Recent Activity

Nov 21 2019

guiandrade created D70574: [lldb-server] Verify 'g' is supported before relying on them.
Nov 21 2019, 3:52 PM · Restricted Project

Oct 30 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

This looks fine to me. Thanks for your patience. Do you still need someone to commit this for you?

Oct 30 2019, 9:30 AM · Restricted Project, Restricted Project

Oct 29 2019

guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

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 if 'p' packets aren't supported (it assumes that 'P' also will not).

Oct 29 2019, 1:33 PM · Restricted Project, Restricted Project

Oct 28 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

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 .

Oct 28 2019, 4:40 PM · Restricted Project, Restricted Project
guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.
Oct 28 2019, 4:33 PM · Restricted Project, Restricted Project
guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

Adding tests

Oct 28 2019, 4:30 PM · Restricted Project, Restricted Project

Oct 24 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

Have you had a chance to take a look at the last revision of this?

Oct 24 2019, 6:27 AM · Restricted Project, Restricted Project

Oct 11 2019

guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

Properly separating MPX fix from this change (I guess)

Oct 11 2019, 10:05 AM · Restricted Project, Restricted Project
guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

Moving MPX fix to a parent change

Oct 11 2019, 9:38 AM · Restricted Project, Restricted Project
guiandrade created D68874: [lldb] Fix offset intersection bug between MPX and AVX registers.
Oct 11 2019, 9:28 AM · Restricted Project

Oct 9 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

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 for BND registers to mean the gdb-remote offset

Would that involve hard-coding an extra offset at RegisterInfos_x86_64.h:28 and RegisterInfos_i386.h:32?

I would very much like to avoid adding new members to the RegisterInfo struct. As this stuff is specific to the x86 linux context, I'd like to keep this stuff there, if necessary then via keeping a side-table of "ptrace" offsets for each of the register (though I am hoping there's some better way to achieve that).

change any code which uses it for the OS context offset to do something else (the only place doing something like that should be RegisterContextLinux_x86_64.cpp)

Would that be NativeRegisterContextLinux_x86_64.cpp? If yes, the idea would be to somehow remove that extra factor before the ptrace calls?

Yes, I meant NativeRegisterContextLinux_x86_64. And yes, the idea is to remove that factor, or recompute that the ptrace offset in some other way. This could be a side table, a switch on the register set type, or whatever is the simplest.

Oct 9 2019, 12:23 PM · Restricted Project, Restricted Project
guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

Dissociating the use of g/G packets and trying to address the AVX/MPX offset bug

Oct 9 2019, 12:13 PM · Restricted Project, Restricted Project

Oct 7 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

Thank you for looking into this, @labath

Oct 7 2019, 11:55 AM · Restricted Project, Restricted Project

Sep 16 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

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 those other tests? (in case it's relevant, here's my cmake command cmake ../llvm -G Ninja -DLLVM_ENABLE_PROJECTS='lldb;clang;libcxx')

LLDB tests can be run with the "check-lldb" target. There's also "check-all" which would run *all* tests, but that's usually overkill.

About not tying G and g together, maybe could we remove this branch?

That would be fine for lldb-server, but I am not sure how would that play with other stubs. As I understand it, the main reason this read-all-at-once logic was introduced was to handle stubs which did not implement p/P packets, so we may not be able to just start sending P unconditionally. @clayborg or @jasonmolenda can probably say more about that...

Sep 16 2019, 4:29 PM · Restricted Project, Restricted Project

Sep 13 2019

guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

looks fine to me

Sep 13 2019, 5:59 AM · Restricted Project

Sep 9 2019

guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Maybe we could use the previous workaround until we find something better?

Sep 9 2019, 9:45 AM · Restricted Project
guiandrade updated the diff for D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Falling back to the previous workaround to test EnsureAllDIEsInDeclContextHaveBeenParsed.

Sep 9 2019, 9:41 AM · Restricted Project

Sep 6 2019

guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

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 have a regression in the future. Apparently, with or without the extra calls, that command's output looks the same.

I would prefer to have tests. I was just saying that the code looked good, and it is now time to proceed with finding a way to test.

Sep 6 2019, 9:03 AM · Restricted Project
guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

This looks like a good approach to me. Pavel?

Sep 6 2019, 8:07 AM · Restricted Project

Sep 5 2019

guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

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 way we could possibly test this is with a full scale test via the "target modules dump ast" command. So, the idea would be to run to process to some point, evaluate some expressions, run "target modules dump ast" and check the things are in the output (or that they are *not* there).

It's hard to say what exactly you should be checking for, since the assumption is that we are not changing the behavior here, and so the existing tests should cover that in theory, but the "dump ast" command is a new thing, we don't have too many of tests for it, and so any tests you add will be useful, even if they're not specifically targeting the thing you fix in this patch. If you are able to test the thing that the previous patch fixed in this way, than that would be great though. I'm not 100% sure it can be done, but since (IIUC) the bug/problem was about parsing too many things, I would expect that would manifest itself in some additional entries showing up in the parsed ast.

Sep 5 2019, 2:04 PM · Restricted Project

Sep 4 2019

guiandrade added inline comments to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.
Sep 4 2019, 1:37 PM · Restricted Project
guiandrade updated the diff for D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Migrating to EnsureAllDIEsInDeclContextHaveBeenParsed

Sep 4 2019, 1:28 PM · Restricted Project
guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Thanks for the clarification Greg.

Functionally, this patch seems fine, but I am still wondering if we shouldn't make this more efficient. std::set is not the most memory-efficient structure, and so creating a std::set entry just to store one bit seems wasteful, particularly as there is already a container which uses the context as a key (m_decl_ctx_to_die). Could just shove this bit into the m_decl_ctx_to_die map (probably by changing it to something functionally equivalent to map<DeclContext, pair<bool, vector<DWARFDie>>>)? Given that the sole "raison d´être" of this map is to enable the ParseDeclsForContext functionality, I don't think that having that flag be stored there should be awkward. Quite the opposite, it would remove the awkward back-and-forth between the SymbolFileDWARF and the DWARFASTParsers (symbol file calls ForEachDIEInDeclContext, passing it a callback; then ast parser calls the callback; finally the callback immediately re-enters the parser via GetDeclForUIDFromDWARF) -- instead we could just have the SymbolFileDWARF do ast_parser->PleaseParseAllDiesInThisContextIfTheyHaventBeenParsedAlready(ctx) and have everything happen inside the parser.

So, overall, it seems to me that this way, we could improve the code readability while reducing the time, and avoiding a bigger memory increase. In fact, since we now will not be using the list of dies after they have been parsed once, we could even free up the vector<DWARFDie> after the parsing is complete, and thereby reduce the memory footprint as well. That would be a win-win-win scenario. :)

WDYT?

Great idea, but this idea gave me the idea to use "m_decl_ctx_to_die" as is, and just remove the entry once we have parse all decls. Then we free the memory and don't need a bit. If there is no entry in the m_decl_ctx_to_die map, then ForEachDIEInDeclContext will just not iterate at all?

Sep 4 2019, 1:28 PM · Restricted Project

Sep 3 2019

guiandrade updated the diff for D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Renaming the test

Sep 3 2019, 7:20 PM · Restricted Project
guiandrade added inline comments to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.
Sep 3 2019, 7:20 PM · Restricted Project
guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Thank you so much for the thorough explanation and ideas, @labath and @clayborg.

Sep 3 2019, 4:40 PM · Restricted Project
guiandrade updated the diff for D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.
Sep 3 2019, 4:36 PM · Restricted Project

Aug 30 2019

guiandrade added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Hey guys,

Aug 30 2019, 2:53 PM · Restricted Project
guiandrade created D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.
Aug 30 2019, 2:53 PM · Restricted Project

Aug 22 2019

guiandrade added a comment to D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

Sounds good, thank you!

Aug 22 2019, 5:08 PM · Restricted Project, Restricted Project, Restricted Project

Aug 20 2019

guiandrade updated the diff for D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

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 time trying to implement it in a more proper manner. It's your call, @clayborg.

Aug 20 2019, 8:43 AM · Restricted Project, Restricted Project, Restricted Project

Aug 19 2019

guiandrade updated the diff for D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

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.

Aug 19 2019, 12:34 PM · Restricted Project, Restricted Project, Restricted Project

Aug 17 2019

guiandrade added a comment to D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

Needs a test, but looks good.

Aug 17 2019, 11:50 AM · Restricted Project, Restricted Project, Restricted Project

Aug 16 2019

guiandrade created D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.
Aug 16 2019, 11:09 AM · Restricted Project, Restricted Project, Restricted Project
guiandrade added a comment to D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

This is a follow up on 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!

Aug 16 2019, 11:08 AM · Restricted Project, Restricted Project, Restricted Project

Jul 30 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

Oh, sorry about that. I was relying on ninja check, which runs okay for me.

Jul 30 2019, 2:23 PM · Restricted Project, Restricted Project

Jul 29 2019

guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

Adding support for tablegen generated properties introduced in https://reviews.llvm.org/D65185

Jul 29 2019, 11:13 AM · Restricted Project, Restricted Project

Jul 26 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

Could anyone please merge this for me?

Jul 26 2019, 7:27 AM · Restricted Project, Restricted Project

Jul 25 2019

guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

Adds a test to make sure the client is using 'g' by default.

Jul 25 2019, 9:33 AM · Restricted Project, Restricted Project

Jul 10 2019

guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.
Jul 10 2019, 2:05 PM · Restricted Project, Restricted Project

Jul 9 2019

guiandrade added inline comments to D62931: [lldb-server] Add setting to force 'g' packet use.
Jul 9 2019, 4:30 PM · Restricted Project, Restricted Project
guiandrade updated the diff for D62931: [lldb-server] Add setting to force 'g' packet use.

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 ThreadGDBRemote::CreateRegisterContextForFrame to guarantee the server supports 'g' packet.

Jul 9 2019, 4:27 PM · Restricted Project, Restricted Project

Jun 25 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

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?

Jun 25 2019, 10:48 AM · Restricted Project, Restricted Project

Jun 7 2019

guiandrade added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

There are so many various GDB remote servers and it is hard to say what will work for most people. Might be worth testing this with the setting set to true and false on a complex program single step on mac, linux and android and verifying that stepping speeds don't regress at all. Android tends to have hundreds of threads which usually makes it the slowest of all when stepping. If we don't see regressions, then I am fine with no setting. Every scenario I know of benefits from fewer packets with larger content (macOS, watchOS, linux, android) so I am fine just enabling it. Maybe it would be good to check what GDB does by default as it might tell us, via comments, why they don't use it if they did run into issues.

Jun 7 2019, 9:38 AM · Restricted Project, Restricted Project

Jun 5 2019

guiandrade created D62931: [lldb-server] Add setting to force 'g' packet use.
Jun 5 2019, 1:39 PM · Restricted Project, Restricted Project

May 29 2019

guiandrade added a comment to D62221: [lldb-server][LLGS] Support 'g' packets.

BTW, do you have commit access, or you need someone to commit this for you?

May 29 2019, 8:23 AM · Restricted Project, Restricted Project

May 28 2019

guiandrade updated the diff for D62221: [lldb-server][LLGS] Support 'g' packets.

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

May 28 2019, 5:06 PM · Restricted Project, Restricted Project

May 27 2019

guiandrade updated the diff for D62221: [lldb-server][LLGS] Support 'g' packets.

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 .

May 27 2019, 2:45 PM · Restricted Project, Restricted Project

May 22 2019

guiandrade added a comment to D62221: [lldb-server][LLGS] Support 'g' packets.

Thank you so much for the feedback!

May 22 2019, 5:46 PM · Restricted Project, Restricted Project
guiandrade updated the diff for D62221: [lldb-server][LLGS] Support 'g' packets.

Making this revision be responsible only for adding the 'g' packet handler and tests. Also, I'm starting to implement a stronger test to verify the actual content of the packets returned by the new handler.

May 22 2019, 5:40 PM · Restricted Project, Restricted Project

May 21 2019

guiandrade created D62221: [lldb-server][LLGS] Support 'g' packets.
May 21 2019, 2:55 PM · Restricted Project, Restricted Project