-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Batch requests and extract fn/paths from addr2line (IDFGH-15433) #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aea4537 to
a5736c6
Compare
|
Separate commit to show changes - can squash if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the performance and output formatting of the panic decoder by batching addr2line requests and extracting function names, file paths, and line numbers from the tool output.
- Groups addresses by ELF file to reduce latency
- Parses addr2line output into structured trace entries, including inlined functions
- Replaces individual address decoding with batched processing
Comments suppressed due to low confidence (2)
esp_idf_panic_decoder/pc_address_decoder.py:105
- [nitpick] Since the parameter now expects a list of addresses, consider renaming 'pc_addr' to 'addresses' for improved clarity.
def lookup_pc_address(
self,
pc_addr: List[str],
esp_idf_panic_decoder/pc_address_decoder.py:116
- The previous addr2line command used the '-pfiaC' flags, including the '-p' flag for pretty printing. Verify if the omission of '-p' in the new command is intentional and ensure it does not affect the output format expected for parsing.
cmd = [f'{self.toolchain_prefix}addr2line', '-fiaC', '-e', elf_file, *pc_addr]
Thank you for the update. Please squash commits. The CI failure is not related to these changes, I will fix this ASAP. In the meantime, there are two issues reported by pylint that need attention: |
dobairoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebkat Thank you for the contribution. It is a nice idea. I have left several recommendations in order that the code will be clearer for later maintenance.
1a86f37 to
257cecf
Compare
|
@dobairoland Have implemented your suggestions. The text output remains completely unchanged (which is just like the original output of addr2line with pretty-print enabled): The changes are necessary to allow color formatting of the output: In non pretty-print mode So most of the changes are for parsing this and recreating the pretty-print format. One thing which could simplify understanding of the function output is if instead of returning a list with: [..., ("0x4202c8c9", [location1, inline2, inline3]), ...]It could return [..., ("0x4202c8c9", location1), ("(inlined by)", inline2), ("(inlined by)", inline3), ...] |
dobairoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for accepting my suggestions.
peterdragun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for updates. One for request: @nebkat Can you please rebase your MR? This should unblock the GH action for pre-commit.
c09b24f to
2da2274
Compare
| if path == '??' and is_rom: | ||
| path = 'ROM' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if path == '??' and is_rom: | |
| path = 'ROM' | |
| if func == '??' and path == '??': | |
| continue | |
| if path == '??' and is_rom: | |
| path = 'ROM' |
@peterdragun How about this? No point returning it from here either if there is no useful information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added valid variable to this loop - if any entry for a given address contains a function or path we keep the entire trace, otherwise if none of the entries do then we discard the address.
Probably overkill and not sure if this is even possible but this will ensure that if there was ever a situation like this we would keep the full trace, instead of suggesting that 0x40078000 == func at file.c:
0x40078000: ?? at ??:0
(inlined by) func at file.c:1
(inlined by) ?? at ??:0


Description
The current procedure of invoking
addr2lineindividually for each address is rather inefficient, which causes large delays when there is a long backtrace printed inesp-idf-monitor.This change instead groups addresses by their appropriate ELF file and requests all of them at once. The output is then parsed to map back to the specific input addresses.
In addition to this, the function, path and line are extracted to provide for better formatting in
esp-idf-monitor.Related
espressif/esp-idf-monitor#27
Testing
Tested locally with a variety of inputs.
Checklist
Before submitting a Pull Request, please ensure the following: