Do not treat outOfBandRecords as responses

[trond-snekvik]

Checks incoming MI messages for resultRecords before treating them as
responses. In some cases, the server may issue outOfBandRecords with a
token, but no resultRecords. This can for instance happen when the
server wants to report progress on an event before it's finished.
JLink's target-download command does this, and without this extra check,
we'll go straight to monitor reset before the download is actually
finished.

JLink's target-download events look like this:

{"token":8,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}

Note the token (which matches the target-download command's token), and
the lack of a resultRecord. For my 250kB image, this event is issued 5
times before the resultRecord is issued.

[haneefdm]

Sounds like a serious problem and thanks for submitting this PR. I have a few questions though -- marked in the review.

[haneefdm]

For changes like this can you also try other gdb-servers like OpenOCD. What testing have you done so far?

[trond-snekvik]

Sounds like a serious problem and thanks for submitting this PR. I have a few questions though -- marked in the review.

Just to make sure I didn't miss anything - is this project using an external review tool I'm not aware of? This comment does not have any additional review comments attached to it in GitHub.

I have tested this on various nRF52832 DK boards with both JLink and OpenOCD. Both issue these out of band records when we call target-download, and both issue a single resultRecords response at the end.
OpenOCD's output:

{"token":7,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
{"token":7,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
<repeated 26 more times>
{"token":7,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[["address","0x1d730"],["load-size","279921"],["transfer-rate","521872"],["write-rate","7565"]]}}

JLink's output:

{"token":9,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
{"token":9,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
{"token":9,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
{"token":9,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
{"token":9,"outOfBandRecord":[{"isStream":false,"type":"status","asyncClass":"download","output":[]}]}
{"token":9,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[["address","0x1d730"],["load-size","279936"],["transfer-rate","373248000"],["write-rate","12724"]]}}

This change is backed by the GDB documentation, which states:

The output from GDB/MI consists of zero or more out-of-band records followed, optionally, by a single result record. This result record is for the most recent command.

This doc page also states that passing a token for outOfBandRecords is unnecessary, but allowed:

The token is from the corresponding request. Note that for all async output, while the token is allowed by the grammar and may be output by future versions of GDB for select async output messages, it is generally omitted.

As far as I can tell, this issue doesn't impact the actual debug behavior in most cases. The client isn't enabling mi-async mode, so the GDB servers are likely not actually proceeding with the next command until the previous command is finished anyway, but if anything unexpected is reported in the resultRecords, the client will miss this if it already received an outOfBandRecord.

[haneefdm]

Yes, looks better now.

I have not read/written this code. Did have to fix a couple of things over time. I have a question. At line 143, shouldn't there be an "else { Internal error of some sort }". In other words, we have a token with results but no handler. Defensive programming is okay but can that happen? It can happen for instance getting more than one response, but can that happen?

I am not saying add the internal error but maybe a comment or if you have the inclination can you look into it? Adding an error could cause a lot of failures once we release the extension if we are wrong. Logging it might be a tradeoff.

[trond-snekvik]

At line 143, shouldn't there be an "else { Internal error of some sort }". In other words, we have a token with results but no handler. Defensive programming is okay but can that happen? It can happen for instance getting more than one response, but can that happen?

Good question. If it doesn't contain any outOfBandRecords, it'll end up in that unhandled log on line 243 in this scenario, I think. This would at least inform the user that something is wrong. I don't know whether this actually happens at all, but to me, it seems like it's better to be forgiving with the GDB server if it does something unexpected like this, since this handler needs to be compatible with so many implementations and versions.

I can add it though, if you think it's worthwhile

[haneefdm]

Thank you @trond-snekvik I wanted your opinion. I will add something to log if it ever happens. Merging now.