debug: use TCL-RPC to fetch results of OpenOCD commands instead of parsing log file #522
Conversation
|
@timsifive could you please take a look? |
|
Note to reviewers:
|
debug/testlib.py
Outdated
| """Write the command to OpenOCD's stdin. Return the output of the | ||
| command, minus the prompt.""" |
There was a problem hiding this comment.
The comment needs to be updated
| raise TestLibError("unexpected re-definition of TCL-RPC port") | ||
| self.tclrpc_port = int(m.group(3)) | ||
|
|
||
| if self.tclrpc_port and (len(self.gdb_ports) > 0): |
There was a problem hiding this comment.
The issue is not introduced by this patch, but I think it worth pointing out. This condition relies on all GDB ports being reported by OpenOCD prior to TCL-RPC port.
In turn this depends on whether the call to init is implicit or explicit:
> openocd ... -c "gdb_port 0" -c "tcl_port 0" -c "telnet_port disabled" |& grep 'Listening'
Info : Listening on port 41587 for tcl connections
Info : Listening on port 40911 for gdb connections
Info : Listening on port 33639 for gdb connections
> openocd ... -c "gdb_port 0" -c "tcl_port 0" -c "telnet_port disabled" -c init |& grep 'Listening'
Info : Listening on port 40575 for gdb connections
Info : Listening on port 38511 for gdb connections
Info : Listening on port 35989 for tcl connections
Currently, this works, since all the configs in debug/targets/RISC-V call init explicitly.
Wouldn't it be better to add some comment regarding this behavior?
There was a problem hiding this comment.
Let's not add anything unnecessary to this patch. It's fairly urgent since without this fix, OpenOCD development is hamstrung. It's better to merge this soon than it is to disable the tests while we add more improvements to this PR.
There was a problem hiding this comment.
I've already addressed this. Just added a comment
debug/testlib.py
Outdated
| m = self.expect( | ||
| rb"(Listening on port (\d+) for gdb connections|" | ||
| rb"tcl server disabled)", | ||
| rb"Listening on port (\d+) for tcl connections)", | ||
| message="Waiting for OpenOCD to start up...") | ||
| if b"gdb" in m.group(1): | ||
| if b"gdb connections" in m.group(1): | ||
| self.gdb_ports.append(int(m.group(2))) | ||
| else: | ||
| elif b"tcl connections" in m.group(1): | ||
| if self.tclrpc_port: | ||
| raise TestLibError("unexpected re-definition of TCL-RPC port") | ||
| self.tclrpc_port = int(m.group(3)) |
There was a problem hiding this comment.
I would suggest using named match groups and avoid searching a sub-string in a match:
m = self.expect(
rb"Listening on port (?P<port>\d+) for (?P<server>(?:gdb)|(?:tcl)) connections",
message="Waiting for OpenOCD to start up...")
if m["server"] == b"gdb":
self.gdb_ports.append(int(m["port"]))
...
There was a problem hiding this comment.
Moreover, since the regular expression has matched, elif can be replaced with else and an assertion, if you like.
There was a problem hiding this comment.
addressed. I haven't replaced elif with else, though
|
Can you address the pylint complaints for this PR? |
Sure! I'm in the middle of it... |
aap-sc
force-pushed
the
aap-sc/unavailable_fixup
branch
from
3e381ec
to
17297a9
Compare
Done. Note: also addressed comments by @en-sc |
|
pylint is still failing. (I'm commenting here because I'm not sure if you get notified about that automatically.) |
…rsing log file Quick and dirty fix for riscv-software-src#520
aap-sc
force-pushed
the
aap-sc/unavailable_fixup
branch
from
17297a9
to
a4d9f97
Compare
My bad. I was in a hurry and submitted an update as soon as I've tested these changes with OpenOCD. Never bothered to check if a new issue is introduced. Addressed. |
To get riscv-software-src/riscv-tests#522, which fixes an intermittent failure.
|
@timsifive, can you please re-run OpenOCD pipelines failed due to this issue: |
|
They're all running now. |
Quick and dirty fix for #520