fix(esp32): stabilize emulator print stream and re-enable cabi demo#1715
fix(esp32): stabilize emulator print stream and re-enable cabi demo#1715luoliwoshang wants to merge 2 commits intogoplus:mainfrom
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 introduces support for the ESP32 embedded target within the project's testing framework. It establishes a baseline for the ESP32 emulator by configuring which tests should be ignored due to known failures or timeouts, ensuring that the test suite can run effectively on this new target while acknowledging current limitations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 adds test ignore baselines for the esp32 target by adding a new configuration block in cl/compile_test.go. The new configuration lists tests that are currently failing or timing out. The change follows an existing pattern, but there is an opportunity to improve the maintainability of this configuration by reducing data redundancy. I've added a comment with a suggestion for a future refactoring.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| ignoreByDir: map[string][]string{ | ||
| "./_testgo": { | ||
| "./_testgo/abimethod", // panic: internal/bytealg selected .s files require plan9asm translation | ||
| "./_testgo/alias", // unexpected output |
There was a problem hiding this comment.
The // unexpected output comments here (and on ~9 other entries) are less informative than the esp32c3-basic equivalents. For example, esp32c3-basic annotates sigsegv as // unexpected output: got "0/main", expected recover nil-pointer message and tpfunc as // unexpected output: type size mismatch (got 8 4 4, expected 16 8 8).
Adding a brief note on what was unexpected would make future triage significantly easier without needing to re-run each test.
| "./_testrt/complex", // unexpected output | ||
| "./_testrt/fprintf", // link error: ld.lld undefined symbol __stderrp | ||
| "./_testrt/hello", // panic: cannot build SSA for packages | ||
| "./_testrt/len", // panic: runtime index out of range |
There was a problem hiding this comment.
The reason // panic: runtime index out of range is ambiguous — is this a compile-time panic in llgo itself or a runtime panic from the emulator? The same tests on esp32c3-basic are categorized as fast fail: build constraints exclude all Go files, suggesting a different failure stage. Consider clarifying, e.g. // llgo panic: runtime index out of range or // emulator runtime panic: index out of range.
| }, | ||
| "./_testlibc": { | ||
| "./_testlibc/atomic", // unexpected output | ||
| "./_testlibc/demangle", // link error: ld.lld unknown argument -Wl,-search_paths_first |
There was a problem hiding this comment.
Minor: the link error comment format differs from the existing esp32c3-basic entries. For example, esp32c3-basic uses link error: ld.lld: error: unknown argument '-Wl,-search_paths_first' while this uses link error: ld.lld unknown argument -Wl,-search_paths_first. Consider adopting the same ld.lld: error: format for consistency and easier grepping across targets.
|
Overall this is solid test infrastructure work. The per-entry reason comments are a great improvement over the existing esp32c3-basic list (which lacks comments on most entries). The structure follows established patterns well, and there are no security or performance concerns. Main suggestion: the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
=======================================
Coverage 93.15% 93.15%
=======================================
Files 48 48
Lines 13350 13350
=======================================
Hits 12436 12436
Misses 727 727
Partials 187 187 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luoliwoshang
force-pushed
the
feat/esp32-emulator-ignore-refresh
branch
from
c15b716 to
8e50a55
Compare
luoliwoshang
changed the title
1 participant
Summary
_impure_ptrand keepsfopenas fallbackprint/printlnthrough a configurable stream and, onbaremetal && esp, switch tostdoutplus%.*sstring emission to avoid thefwrite(stderr)+floatcrash pattern./_demo/c/cabifromignore_esp32c3_basicso CI validates it againValidation
go install ./cmd/llgocd _demo/go/complex && llgo run -target esp32c3-basic -emulator .cd _demo/c/cabi && llgo run -target esp32c3-basic -emulator .