add static stack usage analysis script#257
Conversation
TASK: ISC-300
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new static stack usage analysis tool designed to help identify potential stack overflows in embedded systems. The tool analyzes ELF files, allowing for detailed configuration of thread entry points, stack limits, and complex call graph scenarios. It provides both command-line output and a web-based visualization via Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable static stack usage analysis script. The implementation is well-structured and includes helpful documentation. My review highlights a critical bug in parsing ELF file information that would cause the script to fail, along with suggestions for performance improvements and several corrections to the README file for better clarity and accuracy.
| for line in raw.splitlines(): | ||
| if "GNU_STACK" not in line: | ||
| continue | ||
|
|
||
| m = re.fullmatch(" +GNU_STACK +(0x[0-9a-fA-F]+ ){5}.*", line) | ||
| assert m | ||
| stack_size = int(m.group(1).rstrip(), 16) | ||
| return stack_size |
There was a problem hiding this comment.
The regex used to parse the readelf output is incorrect and will fail. The GNU_STACK program header line contains non-hexadecimal fields (e.g., RWE), which will cause the re.fullmatch to fail and trigger the assertion on line 38. A more robust approach is to split the line and parse the columns to extract the MemSiz value.
| for line in raw.splitlines(): | |
| if "GNU_STACK" not in line: | |
| continue | |
| m = re.fullmatch(" +GNU_STACK +(0x[0-9a-fA-F]+ ){5}.*", line) | |
| assert m | |
| stack_size = int(m.group(1).rstrip(), 16) | |
| return stack_size | |
| for line in raw.splitlines(): | |
| if "GNU_STACK" in line: | |
| parts = line.split() | |
| if len(parts) > 5: | |
| try: | |
| stack_size = int(parts[5], 16) | |
| return stack_size | |
| except ValueError: | |
| pass |
|
|
||
| ## Useful naming conventions | ||
|
|
||
| There are some naming / coding pracitces that can help to define and keep config up to date. |
There was a problem hiding this comment.
There is a typo in this sentence. pracitces should be practices.
| There are some naming / coding pracitces that can help to define and keep config up to date. | |
| There are some naming / coding practices that can help to define and keep config up to date. |
| void * module_nameThread(void *arg) { | ||
| return NULL | ||
| } | ||
|
|
||
| void module_init(void) { | ||
| beginthread(module_nameThread, 4, module_nameStack, sizeof(module_nameStack), NULL) | ||
| } |
There was a problem hiding this comment.
The C code example is missing semicolons at the end of the return statement on line 33 and the beginthread call on line 37.
| void * module_nameThread(void *arg) { | |
| return NULL | |
| } | |
| void module_init(void) { | |
| beginthread(module_nameThread, 4, module_nameStack, sizeof(module_nameStack), NULL) | |
| } | |
| void * module_nameThread(void *arg) { | |
| return NULL; | |
| } | |
| void module_init(void) { | |
| beginthread(module_nameThread, 4, module_nameStack, sizeof(module_nameStack), NULL); | |
| } |
| page-size | ||
| : Platform specyfic page size used for stack size definitions (default `_start` stack size in phoenix is 3 * page-size) | ||
|
|
||
| reserve | ||
| : Additional bytes reserved on stack | ||
|
|
||
| src-base | ||
| : Path to source code root relative to config file. This is used as base for referencing symbols from specyfic files | ||
|
|
||
| src-base | ||
| : Path to source code root relative to config file. This is used as base for referencing symbols from specyfic files |
There was a problem hiding this comment.
There are a couple of issues in this documentation section:
- The
src-baseoption is defined twice (lines 78-79 and 81-82). The duplicate should be removed. - There is a typo:
specyficshould bespecific. This occurs on lines 73, 79, and 82.
| def enhance_call_tree(self): | ||
| super().enhance_call_tree() | ||
|
|
||
| errors = 0 |
There was a problem hiding this comment.
The errors variable is incremented when a symbol resolution fails, but its value is never used. To avoid silently ignoring unresolved symbols, consider logging the total number of errors at the end of this method or raising an exception if any errors occurred.
| children.append(func) | ||
|
|
||
| while children: | ||
| symbol = children.pop(0) |
There was a problem hiding this comment.
Using list.pop(0) for a queue is inefficient due to its O(n) time complexity. For better performance, especially with large call graphs, you should use collections.deque, which provides an O(1) popleft() operation.
To implement this, you would need to:
- Import
dequeat the top of the file:from collections import deque - Initialize
childrenas a deque on line 127:children = deque(c.symbols_by_name[name] for name in entry_points) - Use
popleft()here:symbol = children.popleft()
1 participant
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment