Page MenuHomePhabricator

labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 4 2013, 6:02 AM (409 w, 4 d)

Recent Activity

Thu, Apr 8

labath accepted D98822: [lldb] [Process] Watch for fork/vfork notifications.

ship it

Thu, Apr 8, 8:11 AM · Restricted Project
labath committed rG2ecf928153fc: [lldb/DWARF] Fix a crash parsing invalid dwarf (pr49678) (authored by labath).
[lldb/DWARF] Fix a crash parsing invalid dwarf (pr49678)
Thu, Apr 8, 7:48 AM
labath committed rG1e511bb1be71: [lldb] Re-skip TestVSCode_launch (authored by labath).
[lldb] Re-skip TestVSCode_launch
Thu, Apr 8, 7:38 AM
labath added a comment to D99497: [LLDB] Fix sync issue in TestVSCode_launch.test_progress_events.

The test is still flaky (http://lab.llvm.org:8011/#/builders/68/builds/10124, http://lab.llvm.org:8011/#/builders/68/builds/10118, http://lab.llvm.org:8011/#/builders/68/builds/10113, ...) In fact, I think it has gotten flakier since the timeouts were added... I'm disabling it.

Thu, Apr 8, 7:34 AM · Restricted Project
labath added a comment to D99944: [LLDB] AArch64 PAC elf-core stack unwinder support.

Setting the address size from the register context is a bit awkward. We have one register context per thread, and they all will be competing to set the value (it should always be the same value under normal circumstances, but it still makes for a strange relationship). It would be better if all of this could be contained in the ABI class. The ABI classes have intimate knowledge of the registers anyway, so it would not be unusual for it to access a specific register to get the mask (here I am assuming that the mask is actually stored in a register).

Thu, Apr 8, 6:35 AM
labath added a comment to D99941: [LLDB] Support AArch64 PAC elf-core register read.

Seems reasonable to me. I'm happy if @DavidSpickett is.

Thu, Apr 8, 5:16 AM
labath added a comment to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

This is going to be pretty big, so I'd say we start splitting this up. The client & server bits can go into separate patches, and it would make things simpler if the qSupported emission and parsing also got its own patches, as that is going to be a lot less controversial.

Thu, Apr 8, 5:04 AM
labath added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Thu, Apr 8, 4:44 AM · Restricted Project
labath accepted D99603: [lldb] [client] Support for multiprocess extension.

Yea, I think it should be fine.

Thu, Apr 8, 4:07 AM · Restricted Project
labath added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

@labath, I think I've made all the requested changes.

Thu, Apr 8, 4:05 AM · Restricted Project
labath added a comment to D99603: [lldb] [client] Support for multiprocess extension.

Yeah, that's tricky. For a inconsistent/unsupported response like this, probably the only reasonable thing we can do print some error message, terminate the connection and put the inferior into some terminal state. However, I don't think we have anything like that (and I suspect there are plenty of other ways one could trigger a loop like this). I'd say we should just delete the test right now.

Thu, Apr 8, 3:29 AM · Restricted Project
labath added a comment to D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files..

If gdb does it, then I don't have any issues with this functionality. It could use a test case though. You can try rewriting that gdb test case for lldb -- we don't have fancy dwarf assemblers (cool stuff, btw), we just use asm (you could look at test/Shell/SymbolFile/DWARF/dwo-type-in-main-file.s for inspiration). However, generating the inputs via clang should be fine as well..

Thu, Apr 8, 1:58 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Apr 1

labath committed rGbad5ee15ea2e: [lldb] Make TestLoadUsingLazyBind work on linux (authored by labath).
[lldb] Make TestLoadUsingLazyBind work on linux
Thu, Apr 1, 5:50 AM
labath committed rG48e3da13519d: [lldb] Rewrite TestAutoInstallMainExecutable logic (authored by labath).
[lldb] Rewrite TestAutoInstallMainExecutable logic
Thu, Apr 1, 5:20 AM
labath added a comment to rG68bb51acd572: [lldb] Fix TestAutoInstallMainExecutable.py.

Thanks for jumping in. I was under the impression that I have tried this out on a mac, but I must have been mistaken. When I look at the test now, I'm amazed it even works on linux -- the test uses the debug info from one binary to debug a different one, and it's pure luck that the build variable has the same address in both.

Thu, Apr 1, 5:15 AM
labath accepted D98761: Fix "image lookup --address" Summary results for inline functions..
Thu, Apr 1, 1:46 AM · Restricted Project
labath added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

What about debug registers on FreeBSD? This system is pretty unique because child processes inherit dbregs from parent, and therefore we need to clear them. GDB doesn't do that and watchpoints crash forked children. Should we keep the clearing part as a hack specific to FreeBSD, or have lldb generic clear all hardware breakpoints and watchpoints on fork?

Thu, Apr 1, 1:46 AM · Restricted Project
labath added a comment to D99603: [lldb] [client] Support for multiprocess extension.

I think this looks fine. Could you also create a gdb-client test case (you can ignore the default thread thingy)?

Thu, Apr 1, 12:29 AM · Restricted Project
labath committed rGe1d4fb1ebfff: [lldb] Fix build errors from 3bea7306e8 (authored by labath).
[lldb] Fix build errors from 3bea7306e8
Thu, Apr 1, 12:04 AM

Wed, Mar 31

labath committed rG3bea7306e866: [lldb] Fix compilation with gcc-6.5 (authored by labath).
[lldb] Fix compilation with gcc-6.5
Wed, Mar 31, 11:45 PM

Tue, Mar 30

labath committed rG0bbe2a3c8aae: [lldb] More missing includes in TestGdbRemote_vContThreads (authored by labath).
[lldb] More missing includes in TestGdbRemote_vContThreads
Tue, Mar 30, 9:07 AM
labath committed rG9709186681a7: [lldb] Add missing include in TestGdbRemote_vContThreads test (authored by labath).
[lldb] Add missing include in TestGdbRemote_vContThreads test
Tue, Mar 30, 8:39 AM
labath committed rGbbae06652e07: [lldb] Fix TestStopOnSharedlibraryEvents.py on linux (authored by labath).
[lldb] Fix TestStopOnSharedlibraryEvents.py on linux
Tue, Mar 30, 8:39 AM
labath added a comment to rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads.

... and 04b766da should do just that. If darwin suffered from the same problem, then the test will start (x)failing after that.

Tue, Mar 30, 8:04 AM
labath committed rG04b766dab0d9: [lldb/test] Deflake TestGdbRemote_vContThreads even more (authored by labath).
[lldb/test] Deflake TestGdbRemote_vContThreads even more
Tue, Mar 30, 8:03 AM
labath added a comment to rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads.

Ok, *now* I've figured out the issue.

Tue, Mar 30, 7:22 AM
labath committed rGce03a862372a: [lldb] Remove linux/mips debugging support (authored by labath).
[lldb] Remove linux/mips debugging support
Tue, Mar 30, 6:29 AM
labath added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

It includes refactoring clone/fork/vfork event monitoring into a single function. Right now, only forks are supported — the handler creates a local NativeProcessLinux instance (@labath, any suggestions how to make this less hacky?), duplicates parent's breakpoints into it (just like the child gets parent's memory) and uses it to clear these breakpoints and detach the child process.

I don't find this particularly hacky. However, I have doubts about the breakpoint clearing aspect. If we add that now, I think it will be tricky to move that process to the client in the future (how will the client know whether the breakpoints were auto-removed?) Given that breakpoints in forked process have been borked since forever, I don't see harm in keeping that broken for a little while longer. Even a client with extremely limited of multiple processes (like the one we had in mind) should not have a problem with sending the appropriate z packets before it detaches. This patch could still come first (it's great that you've separated that out) -- it just needs to ensure that there are no breakpoints in the child path which would need clearing.

Actually, that's not a problem at all. If client doesn't indicate fork-events support via qSupported, we handle breakpoints server-side. If it does, we deliver fork stop reason and then the client is responsible for dealing with breakpoints.

Tue, Mar 30, 6:22 AM · Restricted Project
labath added a comment to D98761: Fix "image lookup --address" Summary results for inline functions..

I'm not that familiar with this function, but the output seems right. Given that the show_inline_callsite_line_info argument is no longer used, can you remove it?

Tue, Mar 30, 5:31 AM · Restricted Project
labath added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

It includes refactoring clone/fork/vfork event monitoring into a single function. Right now, only forks are supported — the handler creates a local NativeProcessLinux instance (@labath, any suggestions how to make this less hacky?), duplicates parent's breakpoints into it (just like the child gets parent's memory) and uses it to clear these breakpoints and detach the child process.

Tue, Mar 30, 5:15 AM · Restricted Project
labath accepted D96463: [LLDB] Arm64/Linux test case for MTE and Pointer Authentication regset.
Tue, Mar 30, 2:52 AM · Restricted Project
labath accepted D96460: [LLDB] Arm64/Linux Add MTE and Pointer Authentication registers .

Looks good (and sorry for the delay). Just rebase to main to avoid the cast.

Tue, Mar 30, 2:47 AM · Restricted Project
labath committed rGd1486e65a164: [lldb] Change CreateHostNativeRegisterContextLinux argument type (authored by labath).
[lldb] Change CreateHostNativeRegisterContextLinux argument type
Tue, Mar 30, 2:46 AM
labath added a comment to D99497: [LLDB] Fix sync issue in TestVSCode_launch.test_progress_events.

Isn't there a better way to ensure synchronization here? Maybe, executing some command (setting a breakpoint, for instance), that will ensure that all symbols have been parsed?

That is what I attempted do in the test: set a breakpoint at 'main' which should trigger all of the needed events. Then we run to a breakpoint and hit it. and after we hit this breakpoint, then we check the progress events.

Tue, Mar 30, 2:34 AM · Restricted Project
labath added a comment to D98482: [lldb] [server] Support for multiprocess extension.

I think it'd be better to commit this in two patches after all. The first one could include the server bits (and the multiprocess+ server thingy) -- I believe this is good to go already. The second patch/review could deal with the thread/process id business on the client (and finally enable multiprocess+ client-side).

Tue, Mar 30, 2:12 AM · Restricted Project
labath added a reverting change for rG1b96e133cf52: [lldb/DWARF] Simplify DIE extraction code slightly: rG1a2d25fcdd73: Revert "[lldb/DWARF] Simplify DIE extraction code slightly".
Tue, Mar 30, 1:01 AM
labath committed rG1a2d25fcdd73: Revert "[lldb/DWARF] Simplify DIE extraction code slightly" (authored by labath).
Revert "[lldb/DWARF] Simplify DIE extraction code slightly"
Tue, Mar 30, 1:01 AM

Mon, Mar 29

labath added a comment to rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads.

I haven't known that the test is indeed flaky on darwin (I was assuming it fails 100%). It seems like this could be the same/similar issue on both platform then. However, I am completely baffled as to what could that issue be. Right now, I am assuming a kernel bug or a weird libc feature...

Mon, Mar 29, 11:57 PM
labath committed rGea08d4ba3736: [lldb] Remove ScriptInterpreterLuaTest.Plugin unittest (authored by labath).
[lldb] Remove ScriptInterpreterLuaTest.Plugin unittest
Mon, Mar 29, 11:52 PM
labath committed rG5978912da00a: [lldb] Add a dwarf unit test for null unit dies (authored by labath).
[lldb] Add a dwarf unit test for null unit dies
Mon, Mar 29, 11:48 PM
labath committed rG1b96e133cf52: [lldb/DWARF] Simplify DIE extraction code slightly (authored by labath).
[lldb/DWARF] Simplify DIE extraction code slightly
Mon, Mar 29, 11:44 PM
labath accepted D99559: [Docs] Update googletest docs link.
Mon, Mar 29, 11:26 PM · Restricted Project
labath added a comment to D99497: [LLDB] Fix sync issue in TestVSCode_launch.test_progress_events.

Isn't there a better way to ensure synchronization here? Maybe, executing some command (setting a breakpoint, for instance), that will ensure that all symbols have been parsed?

Mon, Mar 29, 11:37 AM · Restricted Project

Fri, Mar 26

labath updated subscribers of rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads.

Ok, I've just seen it fail:

======================================================================
FAIL: test_signal_all_threads_llgs (TestGdbRemote_vContThreads.TestGdbRemote_vContThreads)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mono/lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py", line 186, in expect_lldb_gdbserver_replay
    content = server.get_raw_output_packet()
  File "/mono/lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py", line 960, in get_raw_output_packet
    return self._read(self._output_queue)
  File "/mono/lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py", line 917, in _read
    new_bytes = self._sock.recv(4096)
socket.timeout: timed out
Fri, Mar 26, 1:17 PM
labath added a comment to D97739: Add a progress class that can track and report long running operations that happen in LLDB..

This test is flaky (even after the vdso fix by raphael), and it seems like it could be a genuine problem: http://lab.llvm.org:8011/#/builders/68/builds/9440, http://lab.llvm.org:8011/#/builders/68/builds/9409, http://lab.llvm.org:8011/#/builders/68/builds/9403, http://lab.llvm.org:8011/#/builders/68/builds/9383, ...

Fri, Mar 26, 9:29 AM · Restricted Project
labath committed rG5c3aed98afda: [lldb] Skip TestVSCode_launch.test_progress_events on linux (authored by labath).
[lldb] Skip TestVSCode_launch.test_progress_events on linux
Fri, Mar 26, 9:27 AM
labath committed rG21589d07665c: [lldb] XFAIL TestGdbRemote_vContThreads on macos (authored by labath).
[lldb] XFAIL TestGdbRemote_vContThreads on macos
Fri, Mar 26, 9:27 AM
labath committed rG22e2d117d3b9: [lldb] Really fix dwarf5-debug_line-file-index.s (authored by labath).
[lldb] Really fix dwarf5-debug_line-file-index.s
Fri, Mar 26, 9:27 AM
labath added a comment to rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads.

Are you sure that in the case of macOS, "quite frequently" does not mean "always"? On lldb-cmake http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/, at least, it has failed at least the last 10 builds.

Fri, Mar 26, 8:24 AM

Mon, Mar 22

labath committed rGd9643af11958: [lldb] Re-disable dwarf5-debug_line-file-index.s (authored by labath).
[lldb] Re-disable dwarf5-debug_line-file-index.s
Mon, Mar 22, 8:29 AM
labath added a comment to D98619: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF.

The backtrace was very helpful. Thanks.

Mon, Mar 22, 7:31 AM · Restricted Project
labath committed rG10d54e2f8de1: [lldb] Attempt to fix dwarf5-debug_line-file-index.s (authored by labath).
[lldb] Attempt to fix dwarf5-debug_line-file-index.s
Mon, Mar 22, 7:29 AM
labath committed rG8248dd91d7f0: [lldb] Fix test_exec_root of API tests (authored by labath).
[lldb] Fix test_exec_root of API tests
Mon, Mar 22, 7:17 AM
labath requested changes to D96460: [LLDB] Arm64/Linux Add MTE and Pointer Authentication registers .

This still suffers from the duplication of the auxv reading code from NativeProcessELF. Other than that, it's ok-ish.

Mon, Mar 22, 5:41 AM · Restricted Project
labath added a comment to D98619: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF.

REQUIRES: x86 disables the test in those builds, which don't have the X86 target enabled (in the build config). That is exactly what we want here -- the test is not actually running this code, so it should succeed regardless of the host architecture. It's working fine on aarch64, for instance (http://lab.llvm.org:8011/#/builders/96/builds/5891). I'm not sure what's the deal with the arm bot (cc @omjavaid). It could be some 32-bit thing. I'm going to try to get a better error message first, and then we'll see what to do next...

This is crashing LLDB on arm linux I have reported a bug and marked it xfail. I ll look for a fix later.

Mon, Mar 22, 5:34 AM · Restricted Project
labath added a comment to D98482: [lldb] [server] Support for multiprocess extension.

This should be fine, assuming the following statement is true: "all thread id's that we're passing from server to client are in the form of some lldb-specific extension to the gdb-remote protocol". If that is not the case, then we should also update the client to work with the new format.

That statement is somewhat confusing. Do I understand correctly that I should update the client to handle process IDs in all the places where GDB protocol permits server to send PIDs? I suppose that makes sense.

Mon, Mar 22, 5:30 AM · Restricted Project
labath accepted D98482: [lldb] [server] Support for multiprocess extension.

This should be fine, assuming the following statement is true: "all thread id's that we're passing from server to client are in the form of some lldb-specific extension to the gdb-remote protocol". If that is not the case, then we should also update the client to work with the new format.

Mon, Mar 22, 3:55 AM · Restricted Project
labath added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

The gdb model - since gdb only supports one debugee per gdb - is to either follow the fork or follow the parent. It would be more in keeping with lldb's model to make a new target for the child side of the fork, and use that to follow the child. That way you can continue to debug both the parent and the child processes. It doesn't look like you've gotten that far yet, but IMO that's the direction we should be going for lldb.

Mon, Mar 22, 3:18 AM · Restricted Project
labath accepted D98942: [cmake] Disable GCC 9's -Wpessimizing-move.

looks good to me.

Mon, Mar 22, 2:36 AM · Restricted Project
labath accepted D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

I very happy with how this has turned out. Thanks for your patience.

Mon, Mar 22, 2:27 AM · Restricted Project
labath updated subscribers of D98619: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF.

REQUIRES: x86 disables the test in those builds, which don't have the X86 target enabled (in the build config). That is exactly what we want here -- the test is not actually running this code, so it should succeed regardless of the host architecture. It's working fine on aarch64, for instance (http://lab.llvm.org:8011/#/builders/96/builds/5891). I'm not sure what's the deal with the arm bot (cc @omjavaid). It could be some 32-bit thing. I'm going to try to get a better error message first, and then we'll see what to do next...

Mon, Mar 22, 1:42 AM · Restricted Project
labath added a comment to D96626: Support: mapped_file_region: Pass MAP_NORESERVE to mmap.

[Disclaimer: I also don't consider myself to be the owner of this code.]

Mon, Mar 22, 1:33 AM · Restricted Project
labath added a comment to D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions.

Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If I follow this right, then the final value type will be determined by the last operand. That sounds like it could be right for regular dwarf expressions, but I'm not sure about those with pieces. The classification function will mark those as "memory", and that doesn't sound right...

Mon, Mar 22, 1:06 AM · Restricted Project
labath added a comment to D98619: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF.

Thanks a lot for the review Pavel! I've updated the test. If it looks fine like this, could you help me to commit this change?

Mon, Mar 22, 12:50 AM · Restricted Project
labath committed rG68dafe40a69f: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF (authored by kimanh).
[lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF
Mon, Mar 22, 12:46 AM
labath closed D98619: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF.
Mon, Mar 22, 12:45 AM · Restricted Project

Thu, Mar 18

labath added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

Seems mostly ok. The getPidForTid business does not seem completely ideal, though. I think it may be cleaner to keep the threads for which we have received the creation event in some kind of a purgatory list until we receive the corresponding parent event (which will clarify what kind of a child we are dealing with).

Thu, Mar 18, 1:24 PM · Restricted Project
labath committed rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads (authored by labath).
[lldb] Fix flakyness in TestGdbRemote_vContThreads
Thu, Mar 18, 12:42 PM
labath committed rG68bb51acd572: [lldb] Fix TestAutoInstallMainExecutable.py (authored by labath).
[lldb] Fix TestAutoInstallMainExecutable.py
Thu, Mar 18, 7:21 AM

Wed, Mar 17

labath added a comment to D98482: [lldb] [server] Support for multiprocess extension.

Create a new GDBRemoteError class to pass gdb-remote $E codes through cleanly.

The error codes we use right now are completely meaningless, so don't bother preserving them. I don't think we should be introducing a separate error class on their account, and I'm particularly unhappy about how this class has insinuated itself into the Status object.

Wed, Mar 17, 8:09 AM · Restricted Project
labath accepted D98749: [lldb] [test] Fix TestGdbRemote_vContThreads.py logic.
Wed, Mar 17, 7:46 AM · Restricted Project
labath accepted D98619: [lldb] Use CompileUnit::ResolveSymbolContext in SymbolFileDWARF.

I remember seeing this that this (duplicated) code was not dwarf5 ready, but I did not want to change it without understanding when it actually gets used. It looks like you've found that. Thanks for doing that, and for removing the duplication.

Wed, Mar 17, 7:41 AM · Restricted Project

Mon, Mar 15

labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

Right, I see. The contained lists are going to make this trickier than I expected, and might even make the code more complicated. (Though this tradeoff will change if we ever end up with two optional regsets that need to mess with contained lists.)

Mon, Mar 15, 1:40 PM · Restricted Project

Sun, Mar 14

labath committed rG463863fffea5: [lldb] Move PlatformPOSIX::ConnectToWaitingProcesses to RemoteAwarePlatform (authored by labath).
[lldb] Move PlatformPOSIX::ConnectToWaitingProcesses to RemoteAwarePlatform
Sun, Mar 14, 2:44 PM
labath accepted D98589: [llvm] [dwarf] Fix DWARFListTableHeader::getOffsetEntry off-by-one.
Sun, Mar 14, 1:08 PM · Restricted Project
labath added a comment to D98482: [lldb] [server] Support for multiprocess extension.

I'm still bugged by the GetPidTid interface -- the pair of optionals is very complicated. What if we had this function take a default_pid argument (which would be equal to the currently selected process), and it returned that as the process id, in case the packet does not contain one? Then the interface could be something like Optional<pair<pid_t, tid_t>> GetPidTid(pid_t default_pid) (because tid automatically defaults to -1), which I think is more intuitive. In case we really need to differentiate between a pid not being present and an implicitly chosen pid, then the interface could be like Optional<pair<Optional<pid_t>, tid_t>> GetPidTid(), which still seems /slightly/ better as its easier to recognise invalid input. WDYT?

Sun, Mar 14, 1:00 PM · Restricted Project
labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

Thanks. This looks much better, but there is still one thing which bugs me about the register info constructor. Currently, there are three paths through that constructor:

  1. when we don't have any fancy registers (this is the original one)
  2. when we have just SVE (added with the sve support)
  3. when we have pauth et al. (added now)
Sun, Mar 14, 12:37 PM · Restricted Project

Fri, Mar 12

labath added a comment to D98482: [lldb] [server] Support for multiprocess extension.

Some initial remarks:

  • the parsing function is clearly gdb-related, so it should go into StringExtractorGDBRemote.h, and not its base class
  • the universalness of the function makes its interface pretty complicated, and bloats the call sites. Its probably good for the lowest layer, but maybe we could make things a lot cleaner if we created some more specialized functions on top of that. For example, most of the code (packets) statically knows whether it can accept -1 or 0 for process or thread ids, so we would benefit from those checks being done in a central place. in particular, until we really start supporting multiple processes, all of the packets will be doing something like if (pid != m_process->pid()) return error(), so we could have a version that accepts a value that *must* be present as the pid field. That would also allow us to mostly abstract away the protocol differences since the caller could just deal with the tid, knowing that the pid (if present) has already been checked.
  • I wouldn't add multiprocess to the qSupported packets just yet. Since this is accepting the extended syntax regardless of whether the other side claimed to support it (there's no hard in doing that, right?), we could flip the switch after all packets have been ported.
Fri, Mar 12, 11:33 AM · Restricted Project

Mar 11 2021

labath added a reviewer for D98400: [Test][DebugInfo] Check for backend object emission support.: jhenderson.

@jhenderson is probably the best person for this, but my suggestion would be to avoid the need to query both isConfigurationSupported and supportsObjectEmission everywhere. Having the latter subsume the former should make things simpler and still be understandable, I think.

Mar 11 2021, 2:19 AM · Restricted Project
labath added a comment to D97017: [lldb-server] Exit the DataAvailableCallback loop when `done` or `interrupt` are set.

What are the situations where we set done or interrupt? The ones I could find are:

  • the k packet. This packet is (for good reasons) specified very vaguely. The spec says it should have no reply, but lldb will actually try to listen for a reply anyway. I think that making lldb-server stop sending the k reply might be a good thing (for protocol conformance), but that still doesn't mean we should exit immediately after receiving it. The ideal behavior would be to wait(2) for the inferior, and only exit after it has been reaped properly.
  • the interrupt (^C) packet in the platform personality. I'm not sure that we ever actually send this packet on platform connections, but even if we did, exiting does not seem like the proper response.
Mar 11 2021, 2:14 AM
labath committed rG3d47f1f9b826: [lldb] Remove implicit_const_form_support.test (authored by labath).
[lldb] Remove implicit_const_form_support.test
Mar 11 2021, 1:47 AM
labath committed rG075de2d8a756: Save and restore previous terminal after setting the terminal for checking if… (authored by augusto2112).
Save and restore previous terminal after setting the terminal for checking if…
Mar 11 2021, 1:47 AM
labath closed D95230: Save and restore previous terminal after setting the terminal for checking if terminal supports colors..
Mar 11 2021, 1:47 AM · Restricted Project
labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

Yes, definitely.

Mar 11 2021, 1:46 AM · Restricted Project
labath added a comment to D95230: Save and restore previous terminal after setting the terminal for checking if terminal supports colors..

Ownership of various Support components is a little bit unclear, but I think this is sufficiently obvious that I'll just go ahead and commit it for you. Thanks for your patience.

Mar 11 2021, 1:46 AM · Restricted Project
labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

Would something like

do the trick? It needs a bit of cleanup (I haven't actually removed ConfigureRegisterInfos function yet -- I just moved it right next to the reginfo creation), but I think should make it clear where I'm going with this.

Mar 11 2021, 1:14 AM · Restricted Project
labath added a comment to D98289: [lldb] 2/2: Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).

I'm not sure what are the "planned changes", but have you compared this implementation with the one in llvm. IIRC, I tried to implement it the same one as the one over there, but it seems the llvm one has changed since then. At a cursory glance it looks like it does not suffer from this problem, so it would be preferable if we could fix this problem, by making our implementation be more like the llvm one.

Mar 11 2021, 1:07 AM · Restricted Project

Mar 9 2021

labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

ConfigureRegisterContext is called only once in the lifetime of a core from RegisterContextCorePOSIX_arm64 constructor. A process registers cannot change during execution which is what made basis of this change. for NativeRegisterContextLinux_arm64 I moved the ConfigureRegisterInfos logic out of configure register context because it will be called again and again during execution. But for RegisterContextCorePOSIX_arm64 we call it once in the constructor so its safe to keep the function.

Ok, I understand what's happening now -- thanks for re-iterating. I have managed to miss the fact that this patch deals with both core file and live register contexts.

In that case, I'd like to go one step further, and remove the "configuration" operation from the RegisterInfoPOSIX_arm64 class as well (by making it a part of the construction). It should be possible to do that by fetching the information needed to determine the registers before the class is instantiated. For the live context, that could be done inside the CreateHostNativeRegisterContextLinux function. For the core file context, we could create a similar factory function as well. I.e. move the construction of RegisterInfoPOSIX_arm64 from ThreadElfCore into some static function inside RegisterContextCorePOSIX_arm64 -- this function could examine the core data to determine the registers and construct the appropriate info class.

hmm .. in that case we will have to defer creation of RegisterInfoPOSIX_arm64 as you said above. I will create a function that will allow us to update unique_ptr m_register_info_up in RegisterContextPOSIX_arm64

Mar 9 2021, 7:57 AM · Restricted Project
labath added inline comments to D96459: [LLDB] Pull AuxVec info into NativeRegisterContextLinux_arm64.
Mar 9 2021, 7:46 AM
labath requested changes to D96459: [LLDB] Pull AuxVec info into NativeRegisterContextLinux_arm64.
Mar 9 2021, 7:43 AM
labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

ConfigureRegisterContext is called only once in the lifetime of a core from RegisterContextCorePOSIX_arm64 constructor. A process registers cannot change during execution which is what made basis of this change. for NativeRegisterContextLinux_arm64 I moved the ConfigureRegisterInfos logic out of configure register context because it will be called again and again during execution. But for RegisterContextCorePOSIX_arm64 we call it once in the constructor so its safe to keep the function.

Mar 9 2021, 7:02 AM · Restricted Project
labath committed rG2e826088b983: [lldb] Fix a bug in D96779 (shared lib directory logic) (authored by labath).
[lldb] Fix a bug in D96779 (shared lib directory logic)
Mar 9 2021, 6:21 AM
labath added inline comments to D98197: [lldb] Fix DWARF-5 DW_FORM_implicit_const (used by GCC).
Mar 9 2021, 1:14 AM · Restricted Project
labath accepted D98197: [lldb] Fix DWARF-5 DW_FORM_implicit_const (used by GCC).

I guess we should also delete test/Shell/Breakpoint/implicit_const_form_support.test, as it's not very useful (the numbers it checks come from the line table, not from the implicit constants).

I have not deleted it myself; I find non-racy simple tests to be always good as they may catch some completely unrelated regression in the future.

Mar 9 2021, 1:10 AM · Restricted Project
labath added a comment to D96458: [LLDB] Add support for Arm64/Linux dynamic register sets.

I'm sorry, my response times are pretty slow these days.

I'm thinking about this ConfigureRegisterInfos business. I get that the vector length is variable, and so we need to refresh it after every stop. However, the set of enabled extensions seems more static.

Is it possible for that to change during the lifetime of the process? I'd guess not, because otherwise we'd have to also resend the register lists after every stop. And, if that's the case, I'm wondering if there's a way to reflect this static-ness in the code. For example, by doing this setup in the constructor, instead of a late ConfigureRegisterInfos call. IIRC, the register contexts are created when the thread is stopped, so it should be possible to fetch the information about supported registers quite early.

What do you think?

Yes that is doable. I ll move ConfigureRegisterInfos part into constructor.

Mar 9 2021, 1:06 AM · Restricted Project
labath added a comment to D96766: [lldb] [Process/FreeBSD] Introduce mips64 FPU reg support.

Hm.... I'm very tempted to delete the linux mips implementation -- it uses several techniques which are getting in the way of this patch (and also some others in the past), for a lot of those, we now have different/better ways of doing it. On top of that, it's completely unmaintained (last non-nfc change to the file was in 2017, and the author (@nitesh.jain) does not appear to be active anymore.

Hi Labath,

As you notice, we no longer work for MIPS and not sure who is the current maintainer for the same.

Mar 9 2021, 12:25 AM
labath added a comment to D98197: [lldb] Fix DWARF-5 DW_FORM_implicit_const (used by GCC).

Could you simplify the test case? It sounds like all you need is a single global variable (int foo = 47;), without any functions or that stuff. I guess we should also delete test/Shell/Breakpoint/implicit_const_form_support.test, as it's not very useful (the numbers it checks come from the line table, not from the implicit constants). Other than that, this seems fine.

Mar 9 2021, 12:18 AM · Restricted Project

Mar 8 2021

labath accepted D98196: [lldb] Add missing debugserver/lldb-server dependencies to check-lldb.
Mar 8 2021, 11:39 PM · Restricted Project
labath added inline comments to D96202: [lldb/test] Automatically find debug servers to test.
Mar 8 2021, 8:26 AM · Restricted Project

Mar 5 2021

labath added a comment to D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files..

I'm not sure about using target.debug-file-search-paths, and what the changes Pavel is suggesting would entail.

I imagine it would involve calling Symbols::LocateExecutableSymbolFile to locate the dwo file, similar to how we do that for dwp files (see SymbolFileDWARF::GetDwpSymbolFile). (Disclaimer: I have not tried doing this, so I don't know if that function would work out-of-the-box.)
Note that I myself am not convinced that this is the right solution (that function is rather heavy), but it does solve the problem of "how do we let the user specify/choose the location of the dwo files" (answer: put the path in the target.debug-file-search-paths setting), and it would search the cwd and the module directory automatically. And the "heavy-ness" of the function is moderated by the fact that it is a no-op for absolute paths to existing files.

Mar 5 2021, 10:25 AM · Restricted Project, Restricted Project, Restricted Project