Page MenuHomePhabricator

Fix compatibility with OpenOCD debug stub.
ClosedPublic

Authored by vadimcn on Sep 15 2017, 2:47 PM.

Details

Summary

OpenOCD sends register classes as two separate <feature> nodes, fixed parser to process both of them.

OpenOCD returns "l" in response to "qfThreadInfo", so IsUnsupportedResponse() was false and we were ending up without any threads in the process. I think it's reasonable to assume that there's always at least one thread.

Diff Detail

Repository
rL LLVM

Event Timeline

vadimcn created this revision.Sep 15 2017, 2:47 PM
clayborg edited edge metadata.Sep 15 2017, 3:14 PM

The multiple feature fix is fine.

As for the qfThreadInfo fix, do you not have control over the OpenOCD GDB server? I would be nice to modify it to return something valid in response to qfThreadInfo? If you don't control or have access to modify the OpenOCD GDB server, we should at least verify that you sent qfThreadInfo and got "l" as the only response. Seems weird to say we will accept any kind of input.

This is what I see in the log:

<  16> send packet: $qfThreadInfo#bb
<   5> read packet: $l#6c

And here's code that generates it: https://github.com/gnu-mcu-eclipse/openocd/blob/b21ab1d683aaee501d45fe8a509a2043123f16fd/src/rtos/rtos.c#L370

I agree, they should have responded with "not supported", but I've never heard of a system that allows processes without threads, and openocd is pretty popular in embedded, so I think it would make sense to just support this quirk. That's what gdb does anyways...

clayborg accepted this revision.Sep 15 2017, 4:10 PM

As long as everyone agrees that no threads from qfThreadInfo means there is one thread whose thread ID is 1 then this is ok.

This revision is now accepted and ready to land.Sep 15 2017, 4:10 PM

If we get a response of "l" for the "qfThreadInfo", it might be worth trying to send a "H1" packet (select thread ID 1 for subsequent continues and steps) to see if the server knows about thread 1 before proceeding? This patch also broke other platforms that respond with "OK" to the "qfThreadInfo". So we really need to do some extra verification that "l" was the only things received in response to the "qfThreadInfo".

This obvious solution works well for me and seems safe.

auto isEmptyList = response.IsNormalResponse() && response.GetStringRef() == "l";

if ((response.IsUnsupportedResponse() || isEmptyList) ...

I like the solution detailed above by tatyana-krasnukha

Sorry, for not catching this regression! I've checked that attaching to a standard gdbserver still worked, but was not aware of the other scenarios.

Does lldb-server return 'OK' string in response to 'qfThreadInfo'? According to this, 'OK' is not a valid response to qfThreadInfo. (Is there a better reference for gdb-remote protocol?)

Also, Tamas Berghammer has already submitted a fix, does is still not resolve this issue?

Maybe LLDB should use qAttached to determine if there's an active process? OpenOCD seems to implement that one correctly.