trunner: add support for loading tests on armv7r5f-zynqmp-qemu#465
trunner: add support for loading tests on armv7r5f-zynqmp-qemu#465damianloew merged 4 commits intomasterfrom
Conversation
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 significantly expands 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 support for the armv7r5f-zynqmp-qemu target, enabling application loading via GDB. The changes to trunner/tools/gdb.py for handling initial commands are well-implemented. However, I've identified a critical bug in the new target implementation that could lead to a crash, and an opportunity for refactoring to enhance code quality and efficiency. Please see my detailed comments.
Unit Test Results10 459 tests +906 9 797 ✅ +836 55m 14s ⏱️ + 2m 47s Results for commit a06be35. ± Comparison against base commit 6ed92c4. This pull request skips 1 test.♻️ This comment has been updated with latest results. |
damianloew
force-pushed
the
damianloew/load_apps_on_qemu
branch
from
264dd79 to
9a8ab68
Compare
damianloew
force-pushed
the
damianloew/load_apps_on_qemu
branch
from
9a8ab68 to
ba3eb51
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new PloRamAppLoader base class to standardize the process of loading applications into RAM for various targets. Existing app loaders for STM32L4x6 and MCXN947 have been refactored to inherit from this new base class, eliminating redundant code. Crucially, support for a new target, armv7r5f-zynqmp-qemu, has been added, including a dedicated ARMV7R5FPloAppLoader that utilizes GDB for application loading. The GdbInteractive class has also been enhanced to support initial GDB commands. Additionally, several test configurations across libc, mem, sys, and waitpid have been updated to properly exclude or include the armv7r5f-zynqmp-qemu target based on its specific capabilities, such as the lack of fork() or mprotect functionality on NOMMU architectures. A suggestion was made to refactor the application loading logic in ARMV7R5FPloAppLoader to improve readability and efficiency by pre-gathering application metadata before GDB loading and PLO registration.
| with self.gdb.run(): | ||
| offset = 0 | ||
| self.gdb.connect() | ||
|
|
||
| for app in self.apps: | ||
| path = self.gdb.cwd / Path(app.file) | ||
| sz = self._aligned_app_size(path) | ||
| self.gdb.load(path, self.ramdisk_addr + offset) | ||
| offset += sz | ||
|
|
||
| offset = 0 | ||
| for app in self.apps: | ||
| path = self.gdb.cwd / Path(app.file) | ||
| sz = path.stat().st_size | ||
|
|
||
| self.alias(app.file, offset=offset, size=sz) | ||
| self.app("ramdisk", app.file, "ddr", "ddr") | ||
|
|
||
| offset += self._aligned_app_size(path) |
There was a problem hiding this comment.
To improve readability and avoid redundant calculations, you could gather application metadata (like path, size, and aligned size) in a single loop before performing the GDB loading and PLO registration steps. This would make the logic cleaner and slightly more efficient by not re-calculating paths and sizes in the second loop.
apps_meta = []
for app in self.apps:
path = self.gdb.cwd / Path(app.file)
apps_meta.append(
{
"path": path,
"file": app.file,
"size": path.stat().st_size,
"aligned_size": self._aligned_app_size(path),
}
)
with self.gdb.run():
offset = 0
self.gdb.connect()
for meta in apps_meta:
self.gdb.load(meta["path"], self.ramdisk_addr + offset)
offset += meta["aligned_size"]
offset = 0
for meta in apps_meta:
self.alias(meta["file"], offset=offset, size=meta["size"])
self.app("ramdisk", meta["file"], "ddr", "ddr")
offset += meta["aligned_size"]There was a problem hiding this comment.
IMHO too complicated (I don't like 3 for loops instead of 2, calculating path and getting size is not so time consuming to do it 2 times), but I will change sz to aligned_sz in the first loop.
damianloew
force-pushed
the
damianloew/load_apps_on_qemu
branch
from
fdf7dfb to
cbbe58c
Compare
As this is a dummyfs target, test binaries must be explicitly loaded before execution. This change adds that loading step to trunner. JIRA: CI-652
The following targets shared the same interface for loading apps to RAM: - armv8m33-mcxn94x-frdm - armv7m4-stm32l4x6-nucleo - armv7r5f-zynqmp-qemu Base class has been created to avoid code duplication. JIRA: CI-652
damianloew
force-pushed
the
damianloew/load_apps_on_qemu
branch
from
cbbe58c to
031a48a
Compare
|
Only waiting for coremark to be enabled in p-r-project to set timeouts (locally all benchmarks work), above that ready for review |
damianloew
force-pushed
the
damianloew/load_apps_on_qemu
branch
4 times, most recently
from
2c0c662 to
eeeb019
Compare
damianloew
force-pushed
the
damianloew/load_apps_on_qemu
branch
from
eeeb019 to
a06be35
Compare
2 participants
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment