addr2line: type checking, debugging #2434
Conversation
travisdowns
changed the title
scripts/addr2line.py
Outdated
| """Returns the input stream for the process/pipe.""" | ||
| return notNone(self._input_proc.stdin) | ||
|
|
||
|
|
There was a problem hiding this comment.
a single line would be better, IMHO.
There was a problem hiding this comment.
Agreed, this is fixed by black too.
scripts/addr2line.py
Outdated
| self._executable = executable | ||
| self._kallsyms = kallsyms | ||
| self._current_backtrace = [] | ||
| self._current_backtrace : list[tuple[str, str]] = [] |
There was a problem hiding this comment.
nit, might want to put : next to self._current_backtrace , like:
self._current_backtrace: list[tuple[str, str]] = []There was a problem hiding this comment.
good eye, fixed! (even more reason for black :))
scripts/seastar-addr2line
Outdated
| @@ -51,13 +51,16 @@ def read_backtrace(stdin): | |||
| if char == '' or linefeeds > 1: | |||
| break | |||
|
|
|||
| TestResult = dict[str, Any] | |||
| MaybeTestResult = Union[TestResult, None] | |||
There was a problem hiding this comment.
could use Optional[TestResult].
There was a problem hiding this comment.
Good call, done!
travisdowns
force-pushed
the
td-addr2line-debugging
branch
from
eb96f49
to
9752e6c
Compare
scripts/addr2line.py
Outdated
| if self._missing: | ||
| return " ".join([self._binary, address, '\n']) | ||
| # We print a dummy 0x0 address after the address we are interested in | ||
| # which we can look for in _read_address | ||
| self._input.stdin.write(address + '\n0x0\n') | ||
| self._input.stdin.flush() | ||
| self._input().write(address + '\n0x0\n') |
There was a problem hiding this comment.
if _input is a property, we don't need to call it for retrieving the value returned by it, am i right?
There was a problem hiding this comment.
Sorry, this is correctly without () after the 2nd commit in this series, but absorb assigned it to the wrong commit in this case, let me fix that.
There was a problem hiding this comment.
Fixed in db9e2ff.
|
and you might want to rebase this changeset against master HEAD. |
travisdowns
force-pushed
the
td-addr2line-debugging
branch
2 times, most recently
from
db9e2ff
to
e7b2866
Compare
Done in push e7b2866. |
|
@bhalevy please review too |
| self._missing = res == '' | ||
|
|
||
| @property |
There was a problem hiding this comment.
the last nit: can we make similar changes to self._output, so they are symmetric ?
There was a problem hiding this comment.
Thanks for the suggestion, done in most recent push.
Add additional type hints and a few other type related changes to seastar-addr2line and addr2line.py. After this these scripts type check cleanly with pyright strict. --test passes.
This adds the --debug argument to the seastar-addr2line script. --debug will output additional logging/diagnostics to stderr (the primary output goes to stdout), which can be helpful to diagnose what is going on when addr2line does not decode things as expected. Right now the debugging is targeted mostly at the specific flow of decoding the addresses in the child process, but .debug(...) calls can be added anywhere else if it is convenient.
|
@scylladb/seastar-maint hello maintainers, could you help merge this change? |
travisdowns
force-pushed
the
td-addr2line-debugging
branch
from
e7b2866
to
83fb226
Compare
|
@scylladb/seastar-maint hello maintainers, could you help merge this change? |
This series has two changes:
Add additional type hints and a few other type related changes to seastar-addr2line and addr2line.py. After this these scripts type check cleanly with pyright strict.
Add a --debug arg which actives additional explicit debug logging, quite useful for diagnosing what is going on.
./seastar-addr2line --testpasses after these two changes.