Refactor address handling in filter_exception_decoder#419
Conversation
📝 WalkthroughWalkthroughRefactors exception-decoder address handling: Changes
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)
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 |
|
@coderabbitai full review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
✅ 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
🤖 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 803-806: The refactor removed the caller vs return-address
distinction so _prefetch_addresses is passing return addresses (backtrace frames
/ RA) unchanged to addr2line and resolving the instruction after the call;
restore the address normalization by decrementing return-address lookups by 1
before caching/asking addr2line. Update _prefetch_addresses (and other
flat-prefetch call sites referenced) to call a small helper (e.g.,
normalize_lookup_address or adjust_return_addr) that detects non-zero
backtrace/RA frames and returns addr-1, then use that normalized string when
populating _addr_cache and invoking addr2line so file/line/symbol resolution
matches original behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef2ee4f6-d6a8-4397-a81d-4d89263c68e7
📒 Files selected for processing (1)
monitor/filter_exception_decoder.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monitor/filter_exception_decoder.py (1)
803-814:⚠️ Potential issue | 🟠 MajorRestore return-address normalization for backtrace frames and
RA.This refactor removes the only distinction between fault PCs and return addresses, so non-zero backtrace frames and
RAare now sent toaddr2lineunchanged. That resolves the instruction after the call and can shift the reported symbol/file/line. The same regression now affects bothbuild_backtrace()andbuild_register_trace().🔧 Minimal fix sketch
+ def _normalize_lookup_address(self, addr, is_return_addr=False): + if not is_return_addr or self.is_address_ignored(addr): + return addr + return "0x%08x" % max(int(addr, 16) - 1, 0) + - def _prefetch_addresses(self, addrs): - """Pre-populate _addr_cache in batch for a list of address strings. - Addresses are passed as-is to addr2line (no decrement). - """ + def _prefetch_addresses(self, addrs): + """Pre-populate _addr_cache in batch for ``(addr, is_return_addr)`` pairs.""" lookups = [] seen = set() - for addr in addrs: + for addr, is_return_addr in addrs: if self.is_address_ignored(addr): continue - if addr not in seen: - seen.add(addr) - lookups.append(addr) + lookup = self._normalize_lookup_address(addr, is_return_addr) + if lookup not in seen: + seen.add(lookup) + lookups.append(lookup)- def _resolve_address(self, addr): + def _resolve_address(self, addr, is_return_addr=False): """Resolve a single address through firmware ELF, then ROM ELF.""" if self.is_address_ignored(addr): return None, False - lookup = addr + lookup = self._normalize_lookup_address(addr, is_return_addr) int_addr = int(lookup, 16)- self._prefetch_addresses(addresses) + self._prefetch_addresses((addr, j > 0) for j, addr in enumerate(addresses)) @@ - output, is_rom = self._resolve_address(addr) + output, is_rom = self._resolve_address(addr, is_return_addr=(j > 0))- reg_addrs.append(addr) + reg_addrs.append((addr, reg_name == "RA")) @@ - output, _ = self._resolve_address(addr) + output, _ = self._resolve_address(addr, is_return_addr=(reg_name == "RA"))Also applies to: 903-917, 968-976, 1022-1029, 1057-1059
🤖 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 803 - 814, The refactor lost return-address normalization so non-zero backtrace frames and the RA register are sent to addr2line without decrementing, producing off-by-one symbol/line results; update _prefetch_addresses and the code paths used by build_backtrace() and build_register_trace() to detect return addresses (non-zero backtrace frames and the 'RA' register) and subtract 1 from those addresses before adding to lookups (but leave fault PCs unchanged), ensuring is_address_ignored logic still applies and use addr2line with the normalized addresses.
🤖 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 803-814: The refactor lost return-address normalization so
non-zero backtrace frames and the RA register are sent to addr2line without
decrementing, producing off-by-one symbol/line results; update
_prefetch_addresses and the code paths used by build_backtrace() and
build_register_trace() to detect return addresses (non-zero backtrace frames and
the 'RA' register) and subtract 1 from those addresses before adding to lookups
(but leave fault PCs unchanged), ensuring is_address_ignored logic still applies
and use addr2line with the normalized addresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23d94fb5-5663-4851-a084-2f44045d08ad
📒 Files selected for processing (1)
monitor/filter_exception_decoder.py
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monitor/filter_exception_decoder.py (1)
803-812:⚠️ Potential issue | 🟠 MajorRestore return-address normalization for backtrace frames and
RA.This refactor removed the only distinction between fault PCs and return addresses, so non-zero backtrace frames and
RAnow go toaddr2lineunchanged. Those should be resolved asaddr - 1; otherwise the reported file/line can shift to the next instruction or even the next symbol.🔧 Minimal fix sketch
- def _prefetch_addresses(self, addrs): - """Pre-populate _addr_cache in batch for a list of address strings.""" + def _prefetch_addresses(self, addrs): + """Pre-populate _addr_cache in batch for a list of (addr, is_return_addr) pairs.""" lookups = [] seen = set() - for addr in addrs: + for addr, is_return_addr in addrs: if self.is_address_ignored(addr): continue - if addr not in seen: - seen.add(addr) - lookups.append(addr) + lookup = ( + "0x%08x" % max(int(addr, 16) - 1, 0) + if is_return_addr + else addr + ) + if lookup not in seen: + seen.add(lookup) + lookups.append(lookup)- def _resolve_address(self, addr): + def _resolve_address(self, addr, is_return_addr=False): @@ - lookup = addr + lookup = ( + "0x%08x" % max(int(addr, 16) - 1, 0) + if is_return_addr + else addr + )- self._prefetch_addresses(addresses) + self._prefetch_addresses([(addr, i > 0) for i, addr in enumerate(addresses)]) @@ - output, is_rom = self._resolve_address(addr) + output, is_rom = self._resolve_address(addr, is_return_addr=(j > 0))- reg_addrs.append(addr) + reg_addrs.append((addr, reg_name == "RA")) @@ - output, _ = self._resolve_address(addr) + output, _ = self._resolve_address( + addr, is_return_addr=(reg_name == "RA") + )Also applies to: 901-913, 965-973, 1019-1026, 1054-1054
🤖 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 803 - 812, The refactor removed address normalization so return-addresses and non-zero backtrace frames are sent raw to addr2line; restore the old behavior by normalizing addresses before batching/lookups: in _prefetch_addresses (and the other address-collection sites handling backtrace frames / the RA field) detect when an address represents a return-address or a non-zero backtrace frame and subtract 1 (addr - 1) before adding to lookups/_addr_cache while still preserving skipped/ignored addresses; update the logic around is_address_ignored, the code paths that build lookups for backtrace frames and RA (e.g., the sections around the original _prefetch_addresses, and the other blocks noted in the review) so addr2line receives the adjusted address for those cases.
🤖 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 803-812: The refactor removed address normalization so
return-addresses and non-zero backtrace frames are sent raw to addr2line;
restore the old behavior by normalizing addresses before batching/lookups: in
_prefetch_addresses (and the other address-collection sites handling backtrace
frames / the RA field) detect when an address represents a return-address or a
non-zero backtrace frame and subtract 1 (addr - 1) before adding to
lookups/_addr_cache while still preserving skipped/ignored addresses; update the
logic around is_address_ignored, the code paths that build lookups for backtrace
frames and RA (e.g., the sections around the original _prefetch_addresses, and
the other blocks noted in the review) so addr2line receives the adjusted address
for those cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5a3e207-8f81-4e84-86a4-3c10886d8868
📒 Files selected for processing (1)
monitor/filter_exception_decoder.py
|
@coderabbitai Your review suggestion is wrong. Do a deeper analyze why the current changes are working as the should AND will result in more correct!! decoded stuff. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI need to read the actual code changes carefully to understand why removing the 🏁 Script executed: Length of output: 55308
Why removing the
|
1 participant
Checklist:
Summary by CodeRabbit