ship it

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.

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).

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

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.

Yea, I think it should be fine.

In D98822#2666257, @mgorny wrote:

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

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.

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..

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.

In D98822#2658780, @mgorny wrote:

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?

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

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

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

In D98822#2658426, @mgorny wrote:

In D98822#2658252, @labath wrote:

In D98822#2646052, @mgorny wrote:

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.

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?

In D98822#2646052, @mgorny wrote:

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.

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

In D99497#2656600, @clayborg wrote:

In D99497#2656502, @labath wrote:

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.

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).

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...

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?

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

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, ...

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.

The backtrace was very helpful. Thanks.

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

In D98619#2641104, @omjavaid wrote:

In D98619#2640734, @labath wrote:

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.

In D98482#2641041, @mgorny wrote:

In D98482#2640981, @labath wrote:

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.

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.

In D98822#2637871, @jingham wrote:

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.

looks good to me.

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

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...

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

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...

In D98619#2634472, @kimanh wrote:

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?

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).

In D98482#2626237, @mgorny wrote:

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.

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.

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.)

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?

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)

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.

@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.

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.

Yes, definitely.

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.

Would something like

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.

In D96458#2614076, @omjavaid wrote:

In D96458#2613948, @labath wrote:

In D96458#2613894, @omjavaid wrote:

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

In D96458#2613894, @omjavaid wrote:

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.

In D98197#2613227, @jankratochvil wrote:

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.

In D96458#2578760, @omjavaid wrote:

In D96458#2566347, @labath wrote:

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.

In D96766#2599440, @nitesh.jain wrote:

In D96766#2596701, @labath wrote:

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.

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.

In D97786#2603656, @cmtice wrote:

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.