- User Since
- Jun 4 2013, 6:02 AM (409 w, 4 d)
Thu, Apr 8
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.
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..
Thu, Apr 1
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.
I think this looks fine. Could you also create a gdb-client test case (you can ignore the default thread thingy)?
Wed, Mar 31
Tue, Mar 30
... 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.
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?
Looks good (and sorry for the delay). Just rebase to main to avoid the cast.
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).
Mon, Mar 29
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?
Fri, Mar 26
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.
Mon, Mar 22
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.
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.
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...
Thu, Mar 18
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).
Wed, Mar 17
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.
Mon, Mar 15
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.)
Sun, Mar 14
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:
- when we don't have any fancy registers (this is the original one)
- when we have just SVE (added with the sve support)
- when we have pauth et al. (added now)
Fri, Mar 12
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.
Mar 11 2021
@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.
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 likedo 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.
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 9 2021
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 8 2021
Mar 5 2021
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.