exec decoder: add support for Stack memory style#408
Closed
Jason2866 wants to merge 10 commits into
Closed
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughExtended the exception decoder to handle stack memory dump lines alongside existing PC:SP backtrace lines. Added pattern recognition for stack memory dumps, a new decoding method for multiple addresses, and routing logic to distinguish between the two backtrace formats. Changes
Sequence DiagramsequenceDiagram
participant Input as Input Line
participant Detector as Pattern Detector
participant Router as Routing Logic
participant StackDecoder as build_stack_trace()
participant BtDecoder as build_backtrace()
participant Output as Decoded Output
Input->>Detector: Exception line
Detector->>Detector: Check BACKTRACE_KEYWORDS
alt Is Stack Memory Line
Detector->>Router: Matched STACK_MEM_LINE
Router->>StackDecoder: Route to stack decoder
StackDecoder->>StackDecoder: Extract addresses<br/>Decode with ROM ELF
StackDecoder->>Output: Decoded entries with prefix
else Is PC:SP Backtrace
Detector->>Router: Matched PC:SP pattern
Router->>BtDecoder: Route to backtrace decoder
BtDecoder->>Output: Decoded backtrace entry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
452-508: Cache repeated addresses within a stack line to reduce subprocess churn.Repeated words in stack dumps currently re-run
addr2linefor the same address, which adds avoidable latency.Suggested refactor
trace = "" + decoded_cache = {} try: for addr in addresses: if self.is_address_ignored(addr): continue - output = self.decode_address(addr, self.firmware_path) - is_rom = False - - if output is None and self.rom_elf_path: - output = self.decode_address(addr, self.rom_elf_path) - if output is not None: - is_rom = True + if addr in decoded_cache: + output, is_rom = decoded_cache[addr] + else: + output = self.decode_address(addr, self.firmware_path) + is_rom = False + if output is None and self.rom_elf_path: + output = self.decode_address(addr, self.rom_elf_path) + if output is not None: + is_rom = True + decoded_cache[addr] = (output, is_rom)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 452 - 508, The build_stack_trace method re-invokes decode_address (which calls addr2line) for duplicate addresses in the same line; add a per-call cache (e.g., a dict) at the start of build_stack_trace to map address -> decoded output (including a marker for ROM vs non-ROM and None for failed decodes), consult this cache before calling decode_address or trying rom_elf_path/firwmare_path, and store the final processed value (after strip_project_dir and ROM labeling) so repeated addresses reuse the cached string and avoid duplicate subprocess calls; keep using is_address_ignored, PREFIX_RE, strip_project_dir, firmware_path and rom_elf_path as currently referenced and return the same formatted trace lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 49-52: The STACK_MEM_LINE regex in
monitor/filter_exception_decoder.py only matches dump lines that start at column
0; update the re.compile for STACK_MEM_LINE to allow optional leading
indentation by permitting whitespace at the start (e.g., change the anchor from
^ to ^\s*), keeping the rest of the pattern identical so indented stack-memory
lines like " 3fca0000: 0x3fce0000 ..." are matched; update the re.compile
call for STACK_MEM_LINE accordingly.
---
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 452-508: The build_stack_trace method re-invokes decode_address
(which calls addr2line) for duplicate addresses in the same line; add a per-call
cache (e.g., a dict) at the start of build_stack_trace to map address -> decoded
output (including a marker for ROM vs non-ROM and None for failed decodes),
consult this cache before calling decode_address or trying
rom_elf_path/firwmare_path, and store the final processed value (after
strip_project_dir and ROM labeling) so repeated addresses reuse the cached
string and avoid duplicate subprocess calls; keep using is_address_ignored,
PREFIX_RE, strip_project_dir, firmware_path and rom_elf_path as currently
referenced and return the same formatted trace lines.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 475-486: The loop calling decode_address for each addr can spawn
many addr2line subprocesses; add a shared cache (e.g., self._addr_decode_cache)
keyed by (addr, elf_path) or just addr when ELF choice is deterministic, and
check it before calling decode_address. In the block that currently iterates
addresses, first skip ignored addresses via is_address_ignored, then look up the
cached result for (addr, self.firmware_path); if missing, call decode_address
and store the result; if result is None and rom_elf_path is set, repeat the
cache lookup/call for (addr, self.rom_elf_path) and mark is_rom accordingly.
Ensure the cache is initialized on the instance (e.g., in __init__) so repeated
addresses reuse previous decode results.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
466-574: Consider extracting shared decode logic to reduce duplication.
build_register_trace,build_stack_trace, andbuild_backtraceshare nearly identical decode-and-format logic: decode from firmware ELF → fallback to ROM ELF → strip project dir → format "in ROM" suffix. Extracting a helper method would consolidate this pattern and make future changes (e.g., adding caching) easier.♻️ Example helper extraction
def _decode_with_rom_fallback(self, addr): """Decode address, trying firmware ELF then ROM ELF. Returns: tuple: (decoded_output, is_rom) or (None, False) if not found """ output = self.decode_address(addr, self.firmware_path) is_rom = False if output is None and self.rom_elf_path: output = self.decode_address(addr, self.rom_elf_path) if output is not None: is_rom = True if output is not None: output = self.strip_project_dir(output) if is_rom: parts = output.split(" at ", 1) output = f"{parts[0]} in ROM" if len(parts) == 2 else f"{output} in ROM" return output, is_romThen each
build_*_tracemethod simply callsself._decode_with_rom_fallback(addr).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 466 - 574, build_register_trace and build_stack_trace duplicate the decode/fallback/strip/ROM-format logic; extract that logic into a single helper (e.g., _decode_with_rom_fallback) that accepts an address and returns the final decoded string or None (or a tuple (output, is_rom) if you prefer), using existing methods/attributes decode_address, firmware_path, rom_elf_path, and strip_project_dir; then replace the core loop body in build_register_trace, build_stack_trace (and build_backtrace if present) to call this helper, skip when it returns None, and append the returned formatted output to trace, leaving subprocess.CalledProcessError handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 568-572: The except subprocess.CalledProcessError block is
unreachable because decode_address() already handles CalledProcessError; remove
this redundant try/except (the handler that logs "%s: failed to call %s: %s\n"
using self.__class__.__name__ and self.addr2line_path) to avoid dead code, or
alternatively move error handling into decode_address() if you intended central
handling — update the code around the call site (the method containing this
except) and references to decode_address() and build_register_trace to rely on
decode_address()'s internal error handling instead.
- Around line 511-515: The except subprocess.CalledProcessError handler is dead
because decode_address() already catches that exception and returns None; remove
the unreachable except block that writes to sys.stderr (the block referencing
subprocess.CalledProcessError and self.addr2line_path) and instead rely on
decode_address() returning None (or handle the None return where appropriate);
search for decode_address() and the surrounding try/except to delete only the
specific except subprocess.CalledProcessError handler so no other error handling
is removed.
---
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 466-574: build_register_trace and build_stack_trace duplicate the
decode/fallback/strip/ROM-format logic; extract that logic into a single helper
(e.g., _decode_with_rom_fallback) that accepts an address and returns the final
decoded string or None (or a tuple (output, is_rom) if you prefer), using
existing methods/attributes decode_address, firmware_path, rom_elf_path, and
strip_project_dir; then replace the core loop body in build_register_trace,
build_stack_trace (and build_backtrace if present) to call this helper, skip
when it returns None, and append the returned formatted output to trace, leaving
subprocess.CalledProcessError handling unchanged.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
monitor/filter_exception_decoder.py (2)
519-523:⚠️ Potential issue | 🟡 MinorUnreachable exception handler — dead code.
The
subprocess.CalledProcessErrorexception is already caught insidedecode_address()(line 470), which returnsNoneon failure. Thistry/exceptblock will never catch that exception and should be removed.🧹 Proposed fix: remove dead code
trace = "" - try: - for reg_name, addr in reg_matches: - if self.is_address_ignored(addr): - continue + for reg_name, addr in reg_matches: + if self.is_address_ignored(addr): + continue - output = self.decode_address(addr, self.firmware_path) - is_rom = False + output = self.decode_address(addr, self.firmware_path) + is_rom = False - if output is None and self.rom_elf_path: - output = self.decode_address(addr, self.rom_elf_path) - if output is not None: - is_rom = True + if output is None and self.rom_elf_path: + output = self.decode_address(addr, self.rom_elf_path) + if output is not None: + is_rom = True - if output is None: - continue + if output is None: + continue - output = self.strip_project_dir(output) + output = self.strip_project_dir(output) - if is_rom: - parts = output.split(" at ", 1) - if len(parts) == 2: - output = f"{parts[0]} in ROM" - else: - output = f"{output} in ROM" + if is_rom: + parts = output.split(" at ", 1) + if len(parts) == 2: + output = f"{parts[0]} in ROM" + else: + output = f"{output} in ROM" - trace += "%s %s: %s: %s\n" % (prefix, reg_name, addr, output) - - except subprocess.CalledProcessError as e: - sys.stderr.write( - "%s: failed to call %s: %s\n" - % (self.__class__.__name__, self.addr2line_path, e) - ) + trace += "%s %s: %s: %s\n" % (prefix, reg_name, addr, output) return trace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 519 - 523, Remove the dead except block that catches subprocess.CalledProcessError in the function that calls decode_address(): decode_address() already catches CalledProcessError and returns None, so delete the outer "except subprocess.CalledProcessError as e:" clause (and its sys.stderr.write call that references self.__class__.__name__ and self.addr2line_path) to avoid unreachable code and simplify the caller.
576-580:⚠️ Potential issue | 🟡 MinorSame unreachable exception handler as in
build_register_trace.This
try/exceptblock is also dead code for the same reason —decode_address()handles the exception internally and returnsNone.🧹 Proposed fix: remove dead code
trace = "" - try: - for addr in addresses: - if self.is_address_ignored(addr): - continue + for addr in addresses: + if self.is_address_ignored(addr): + continue - output = self.decode_address(addr, self.firmware_path) - is_rom = False + output = self.decode_address(addr, self.firmware_path) + is_rom = False - if output is None and self.rom_elf_path: - output = self.decode_address(addr, self.rom_elf_path) - if output is not None: - is_rom = True + if output is None and self.rom_elf_path: + output = self.decode_address(addr, self.rom_elf_path) + if output is not None: + is_rom = True - if output is None: - continue + if output is None: + continue - output = self.strip_project_dir(output) + output = self.strip_project_dir(output) - if is_rom: - parts = output.split(" at ", 1) - if len(parts) == 2: - output = f"{parts[0]} in ROM" - else: - output = f"{output} in ROM" + if is_rom: + parts = output.split(" at ", 1) + if len(parts) == 2: + output = f"{parts[0]} in ROM" + else: + output = f"{output} in ROM" - trace += "%s %s: %s\n" % (prefix, addr, output) - - except subprocess.CalledProcessError as e: - sys.stderr.write( - "%s: failed to call %s: %s\n" - % (self.__class__.__name__, self.addr2line_path, e) - ) + trace += "%s %s: %s\n" % (prefix, addr, output) return trace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 576 - 580, The except block catching subprocess.CalledProcessError is dead code because decode_address() already handles that exception and returns None; remove the unreachable except handler (the subprocess.CalledProcessError except that writes to sys.stderr) in the same try/except around the call to decode_address() in filter_exception_decoder.py (the same pattern used in build_register_trace), leaving the call/flow intact and ensuring no references to the removed e variable or orphaned sys.stderr.write remain.
🧹 Nitpick comments (2)
monitor/filter_exception_decoder.py (2)
527-582: Consider extracting shared decode logic.The methods
build_stack_trace,build_register_trace, andbuild_backtraceshare nearly identical decode-and-format logic (try firmware ELF → try ROM ELF → strip project dir → format ROM suffix). A small helper could reduce duplication.♻️ Example helper extraction
def _decode_and_format(self, addr): """Decode address and return (output, is_rom) or (None, False).""" output = self.decode_address(addr, self.firmware_path) is_rom = False if output is None and self.rom_elf_path: output = self.decode_address(addr, self.rom_elf_path) if output is not None: is_rom = True if output is None: return None, False output = self.strip_project_dir(output) if is_rom: parts = output.split(" at ", 1) output = f"{parts[0]} in ROM" if len(parts) == 2 else f"{output} in ROM" return output, is_rom🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 527 - 582, Extract the duplicated decode-and-format logic from build_stack_trace, build_register_trace, and build_backtrace into a single helper (e.g. _decode_and_format(self, addr)) that tries decode_address(addr, self.firmware_path), falls back to self.rom_elf_path, calls strip_project_dir on successful output, and returns the formatted output (with the "in ROM" handling) or None when unresolved; then replace the repeated blocks in build_stack_trace, build_register_trace, and build_backtrace to call this helper and only handle prefix/line formatting and ignored-address checks.
445-472: Caching implementation looks good; consider minor style adjustment.The cache keying by
(addr, elf_path)correctly distinguishes firmware vs ROM lookups. Static analysis (TRY300) suggests moving the success return to anelseblock for clarity, but this is a minor style preference.♻️ Optional: move success return to else block
try: output = ( subprocess.check_output(args) .decode(enc) .strip() ) # Newlines happen with inlined methods output = output.replace("\n", "\n ") # Check if address was found in ELF (handle common variants) if output in ("?? ??:0", "??:0") or output.strip().startswith("?? ") or output.strip() == "??": self._addr_cache[cache_key] = None return None - - self._addr_cache[cache_key] = output - return output except subprocess.CalledProcessError: self._addr_cache[cache_key] = None return None + else: + self._addr_cache[cache_key] = output + return output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 445 - 472, The try/except currently returns the successful decoded output from inside the try block; for clarity follow TRY300 by using a try/except/else pattern: perform subprocess.check_output and decoding inside try (raising CalledProcessError on failure), handle errors in except (set self._addr_cache[cache_key] = None and return None), and move the success caching/return logic into an else clause that sets self._addr_cache[cache_key] = output and returns output; reference the variables/functions cache_key, self._addr_cache, self.addr2line_path, subprocess.check_output, and the output processing/validation logic so only the return/caching flow is refactored into an else block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 519-523: Remove the dead except block that catches
subprocess.CalledProcessError in the function that calls decode_address():
decode_address() already catches CalledProcessError and returns None, so delete
the outer "except subprocess.CalledProcessError as e:" clause (and its
sys.stderr.write call that references self.__class__.__name__ and
self.addr2line_path) to avoid unreachable code and simplify the caller.
- Around line 576-580: The except block catching subprocess.CalledProcessError
is dead code because decode_address() already handles that exception and returns
None; remove the unreachable except handler (the subprocess.CalledProcessError
except that writes to sys.stderr) in the same try/except around the call to
decode_address() in filter_exception_decoder.py (the same pattern used in
build_register_trace), leaving the call/flow intact and ensuring no references
to the removed e variable or orphaned sys.stderr.write remain.
---
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 527-582: Extract the duplicated decode-and-format logic from
build_stack_trace, build_register_trace, and build_backtrace into a single
helper (e.g. _decode_and_format(self, addr)) that tries decode_address(addr,
self.firmware_path), falls back to self.rom_elf_path, calls strip_project_dir on
successful output, and returns the formatted output (with the "in ROM" handling)
or None when unresolved; then replace the repeated blocks in build_stack_trace,
build_register_trace, and build_backtrace to call this helper and only handle
prefix/line formatting and ignored-address checks.
- Around line 445-472: The try/except currently returns the successful decoded
output from inside the try block; for clarity follow TRY300 by using a
try/except/else pattern: perform subprocess.check_output and decoding inside try
(raising CalledProcessError on failure), handle errors in except (set
self._addr_cache[cache_key] = None and return None), and move the success
caching/return logic into an else clause that sets self._addr_cache[cache_key] =
output and returns output; reference the variables/functions cache_key,
self._addr_cache, self.addr2line_path, subprocess.check_output, and the output
processing/validation logic so only the return/caching flow is refactored into
an else block.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
452-472: Consider movingreturn outputto anelseblock.The static analysis tool suggests using an
elseclause to separate success logic from exception handling, improving clarity.♻️ Optional style improvement
try: output = ( subprocess.check_output(args) .decode(enc) .strip() ) # Newlines happen with inlined methods output = output.replace("\n", "\n ") # Check if address was found in ELF (handle common variants) if output in ("?? ??:0", "??:0") or output.strip().startswith("?? ") or output.strip() == "??": self._addr_cache[cache_key] = None return None self._addr_cache[cache_key] = output - return output except subprocess.CalledProcessError: self._addr_cache[cache_key] = None return None + else: + return output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 452 - 472, The success path inside the try block (where subprocess.check_output(args) is decoded, newlines adjusted, ELF-address checks run, and self._addr_cache[cache_key] is set and returned) should be moved into a try/else structure so that the return output is executed only in the else branch; keep the subprocess.check_output(args) call inside try, catch subprocess.CalledProcessError in except to set self._addr_cache[cache_key]=None and return None, and put the remainder of the post-processing and the self._addr_cache/return logic into the else block (referencing variables/functions: subprocess.check_output, args, enc, output, cache_key, self._addr_cache, subprocess.CalledProcessError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 585-588: The backtrace formatting duplicates " in " for ROM
addresses because _resolve_address sometimes returns output like "function_name
in ROM"; update the trace assembly in the block that calls _resolve_address (the
variables prefix, i, addr, output, and the boolean is_rom from _resolve_address)
to avoid double " in ". Specifically, call _resolve_address(addr) and inspect
its is_rom flag (or detect " in ROM" in output) and format the trace line
differently for ROM entries (e.g., "%s #%-2d %s in %s\n" for normal entries and
"%s #%-2d %s %s\n" or "%s #%-2d %s (%s)\n" for ROM entries) so you do not
append an extra " in " when output already contains "in ROM".
---
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 452-472: The success path inside the try block (where
subprocess.check_output(args) is decoded, newlines adjusted, ELF-address checks
run, and self._addr_cache[cache_key] is set and returned) should be moved into a
try/else structure so that the return output is executed only in the else
branch; keep the subprocess.check_output(args) call inside try, catch
subprocess.CalledProcessError in except to set self._addr_cache[cache_key]=None
and return None, and put the remainder of the post-processing and the
self._addr_cache/return logic into the else block (referencing
variables/functions: subprocess.check_output, args, enc, output, cache_key,
self._addr_cache, subprocess.CalledProcessError).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
533-536: Consider narrowingis_return_addrlogic for register decoding.The current logic
is_return_addr=(reg_name != "MEPC")applies return-address adjustment (subtracting 1) to all registers except MEPC. While this works for RA (return address register), most other registers (SP, GP, A0-A7, etc.) are not return addresses and shouldn't need adjustment.In practice this is harmless since non-code addresses won't resolve anyway, but for correctness you could narrow this:
- output, _ = self._resolve_address(addr, is_return_addr=(reg_name != "MEPC")) + output, _ = self._resolve_address(addr, is_return_addr=(reg_name == "RA"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 533 - 536, The code incorrectly treats every register except "MEPC" as a return address when calling _resolve_address; change the is_return_addr argument in the loop to only true for the actual return address register (e.g., reg_name == "RA") so only RA gets the subtract-one adjustment; update the call in the loop that invokes self._resolve_address(addr, is_return_addr=...) (the loop iterating over reg_matches and using reg_name) to use is_return_addr=(reg_name == "RA").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 533-536: The code incorrectly treats every register except "MEPC"
as a return address when calling _resolve_address; change the is_return_addr
argument in the loop to only true for the actual return address register (e.g.,
reg_name == "RA") so only RA gets the subtract-one adjustment; update the call
in the loop that invokes self._resolve_address(addr, is_return_addr=...) (the
loop iterating over reg_matches and using reg_name) to use
is_return_addr=(reg_name == "RA").
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
107-107: Bound the decode cache to avoid long-session memory growth.With stack-memory decoding enabled, many unique values can be seen; caching all hits/misses indefinitely can grow
self._addr_cachewithout limit.Suggested fix
class Esp32ExceptionDecoder(DeviceMonitorFilterBase): + MAX_ADDR_CACHE_SIZE = 8192 @@ def decode_address(self, addr, elf_path): @@ + def _cache_set(value): + if len(self._addr_cache) >= self.MAX_ADDR_CACHE_SIZE: + self._addr_cache.clear() + self._addr_cache[cache_key] = value + cache_key = (addr, elf_path) if cache_key in self._addr_cache: return self._addr_cache[cache_key] @@ if output in ("?? ??:0", "??:0") or output.strip().startswith("?? ") or output.strip() == "??": - self._addr_cache[cache_key] = None + _cache_set(None) return None - self._addr_cache[cache_key] = output + _cache_set(output) return output except subprocess.CalledProcessError: - self._addr_cache[cache_key] = None + _cache_set(None) return NoneAlso applies to: 445-447, 464-471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` at line 107, The unbounded dict assigned to self._addr_cache should be replaced with a bounded cache to prevent memory growth during long-running decoding; change the initialization (where self._addr_cache = {}) to a size-limited LRU (e.g., collections.OrderedDict or cachetools.LRUCache) with a configurable max size constant, and update every place that writes to or reads from self._addr_cache (the decode/cache hits/misses sites referenced in this file) to evict the oldest entry when the cache exceeds the max size. Ensure the same bounded cache replacement is applied at the other occurrences mentioned (the other initializations/usages around the later decode functions) so all cache usage is size-limited and configurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 396-397: The decoder currently skips lines with only one
REGISTER_ENTRY match because of the "if len(reg_matches) >= 2" check; update the
logic in the function that calls REGISTER_ENTRY.findall(line) (the block using
the reg_matches variable) to handle len(reg_matches) >= 1: when there's a single
match, extract reg_matches[0] and run the same decode path used for multi-match
lines (or call the same helper that decodes entries), and preserve existing
behavior for multiple matches; ensure any unpacking or indexing that expects two
items is guarded so single-entry lines (e.g., "MEPC : 0x...") are decoded
correctly.
---
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Line 107: The unbounded dict assigned to self._addr_cache should be replaced
with a bounded cache to prevent memory growth during long-running decoding;
change the initialization (where self._addr_cache = {}) to a size-limited LRU
(e.g., collections.OrderedDict or cachetools.LRUCache) with a configurable max
size constant, and update every place that writes to or reads from
self._addr_cache (the decode/cache hits/misses sites referenced in this file) to
evict the oldest entry when the cache exceeds the max size. Ensure the same
bounded cache replacement is applied at the other occurrences mentioned (the
other initializations/usages around the later decode functions) so all cache
usage is size-limited and configurable.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
monitor/filter_exception_decoder.py (2)
156-156: Allow optional leading spaces when detecting reboot lines.
REBOOT_REcurrently requiresRebooting...at column 0. Accepting leading whitespace makes context reset more robust with indented/prefixed monitor output.🔧 Small regex tweak
- REBOOT_RE = re.compile(r"^Rebooting\.\.\.", re.IGNORECASE) + REBOOT_RE = re.compile(r"^\s*Rebooting\.\.\.", re.IGNORECASE)Also applies to: 421-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` at line 156, REBOOT_RE currently only matches "Rebooting..." at column 0; update it to allow optional leading whitespace by changing the pattern to include \s* at the start (e.g., r"^\s*Rebooting\.\.\." with re.IGNORECASE) so indented/prefixed monitor lines are detected, and apply the same tweak to the other reboot-detecting regex constants in this module (the other reboot-related regex variables near REBOOT_RE) to keep behavior consistent.
94-153: Make exception metadata constants immutable to avoid accidental shared-state mutation.
XTENSA_EXCEPTIONS,RISCV_EXCEPTIONS, andNON_CODE_REGISTERSare class-level constants but currently mutable (list/dict/set). Freezing them prevents accidental runtime mutation across instances and clears RUF012 warnings. The codebase only reads from these constants (e.g., line 391), so this refactor is safe.♻️ Proposed hardening diff
import os import re import subprocess import sys import glob +from types import MappingProxyType @@ - XTENSA_EXCEPTIONS = [ + XTENSA_EXCEPTIONS = ( @@ - ] + ) @@ - RISCV_EXCEPTIONS = { + RISCV_EXCEPTIONS = MappingProxyType({ @@ - } + }) @@ - NON_CODE_REGISTERS = { + NON_CODE_REGISTERS = frozenset({ @@ - } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/filter_exception_decoder.py` around lines 94 - 153, XTENSA_EXCEPTIONS, RISCV_EXCEPTIONS, and NON_CODE_REGISTERS are currently mutable (list/dict/set); change them to immutable types to prevent accidental shared-state mutation: convert XTENSA_EXCEPTIONS from list to tuple, convert NON_CODE_REGISTERS from set to frozenset, and make RISCV_EXCEPTIONS an immutable mapping (e.g., wrap the dict with types.MappingProxyType after importing types). Update any imports if needed and ensure all usages (e.g., the read at the current usage site like where the constants are referenced) still work with the new immutable types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Line 156: REBOOT_RE currently only matches "Rebooting..." at column 0; update
it to allow optional leading whitespace by changing the pattern to include \s*
at the start (e.g., r"^\s*Rebooting\.\.\." with re.IGNORECASE) so
indented/prefixed monitor lines are detected, and apply the same tweak to the
other reboot-detecting regex constants in this module (the other reboot-related
regex variables near REBOOT_RE) to keep behavior consistent.
- Around line 94-153: XTENSA_EXCEPTIONS, RISCV_EXCEPTIONS, and
NON_CODE_REGISTERS are currently mutable (list/dict/set); change them to
immutable types to prevent accidental shared-state mutation: convert
XTENSA_EXCEPTIONS from list to tuple, convert NON_CODE_REGISTERS from set to
frozenset, and make RISCV_EXCEPTIONS an immutable mapping (e.g., wrap the dict
with types.MappingProxyType after importing types). Update any imports if needed
and ensure all usages (e.g., the read at the current usage site like where the
constants are referenced) still work with the new immutable types.
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #409 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Checklist:
Summary by CodeRabbit