fix(esp32): restore emulator float output and add regression coverage#1699
fix(esp32): restore emulator float output and add regression coverage#1699luoliwoshang wants to merge 7 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 addresses an issue where floating-point output was not correctly handled by the ESP32 emulator. It re-enables this functionality by adjusting linker flags and including required 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
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
|
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| "libm-fpclassify-" + target + ".a", | ||
| } | ||
|
|
||
| for i, group := range config.Groups { |
There was a problem hiding this comment.
The old guard if i >= len(expectedNames) { return } was removed, but the loop still iterates over config.Groups. If a future change adds a group without updating expectedNames, this will panic with index-out-of-bounds instead of producing a clear test failure.
Consider bounding the loop or adding a length check:
| for i, group := range config.Groups { | |
| for i, group := range config.Groups { | |
| if i >= len(expectedNames) { | |
| t.Errorf("unexpected extra group %d: %s", i, group.OutputFileName) | |
| break | |
| } |
| local expected_lines | ||
|
|
||
| echo "Running: llgo run -a -target=${target} -emulator ${case_dir}" | ||
| if ! raw_output=$(llgo run -a -target="${target}" -emulator "${case_dir}" 2>&1); then |
There was a problem hiding this comment.
run_case_and_compare treats a non-zero exit code as a hard failure, but the existing run_emulator_smoke uses set +e and only validates output. ESP32 emulators often exit non-zero on normal completion (e.g. semihosting halt). This inconsistency could make the new regression test flaky in CI.
Consider matching the tolerance approach from run_emulator_smoke, or documenting why stricter handling is intentional here.
| run_emulator_smoke "esp32" "ESP32" "Hello World" | ||
|
|
||
| echo "" | ||
| echo "=== Regression: ESP32 float output (temporary) ===" |
There was a problem hiding this comment.
The (temporary) label here has no associated issue reference or removal criteria. Consider adding a link to the tracking issue (e.g. the struczero type-assert hang mentioned in the PR description) so this doesn't become stale.
| println(i, f) | ||
|
|
||
| // Keep this case on the float-format path without triggering | ||
| // esp32 type-assert timeout cases tracked separately. |
There was a problem hiding this comment.
Nit: The comment says "without triggering esp32 type-assert timeout cases" but the code uses a plain composite literal — no type assertion is involved. The intent is clear (avoid the struczero-style v.(bar) path), but the phrasing could confuse readers into thinking this code somehow relates to type assertions. Consider rephrasing to make it explicit, e.g.:
// Uses plain composite literal instead of type assertion to avoid
// the esp32 type-assert timeout tracked separately.
|
Review Summary Solid fix — the Main concern: No security or performance issues found. |
![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 enables floating-point output for ESP32 targets by modifying the newlib-esp32 cross-compilation configuration to include libm floating-point classification symbols (s_fpclassify.c, sf_fpclassify.c) in a new compile group. It also adds the --undefined=_printf_float linker flag to the ESP32 target definition. A new Go test case (float-1685) and a shell script utility (run_case_and_compare) are introduced to verify the float output on the ESP32 emulator. A review comment suggests refactoring the extract_last_nonempty_lines function in the test script for improved efficiency and more idiomatic shell usage.
| local n="$1" | ||
| awk -v n="$n" ' | ||
| NF { out[++count] = $0 } | ||
| END { | ||
| if (n <= 0 || count == 0) { | ||
| exit | ||
| } | ||
| start = count - n + 1 | ||
| if (start < 1) { | ||
| start = 1 | ||
| } | ||
| for (i = start; i <= count; i++) { | ||
| print out[i] | ||
| } | ||
| }' |
There was a problem hiding this comment.
The extract_last_nonempty_lines function is more complex than necessary and can be inefficient for large inputs as it buffers all non-empty lines in memory. You can achieve the same result more idiomatically and efficiently using a combination of standard shell utilities like grep and tail.
| local n="$1" | |
| awk -v n="$n" ' | |
| NF { out[++count] = $0 } | |
| END { | |
| if (n <= 0 || count == 0) { | |
| exit | |
| } | |
| start = count - n + 1 | |
| if (start < 1) { | |
| start = 1 | |
| } | |
| for (i = start; i <= count; i++) { | |
| print out[i] | |
| } | |
| }' | |
| local n="$1" | |
| # Use grep to filter out empty/whitespace-only lines and tail to get the last n lines. | |
| # This is more memory-efficient than the awk implementation as it doesn't | |
| # buffer the entire input. | |
| grep -v '^[[:space:]]*$' | tail -n "$n" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1699 +/- ##
==========================================
+ Coverage 93.16% 93.37% +0.20%
==========================================
Files 48 48
Lines 13314 13732 +418
==========================================
+ Hits 12404 12822 +418
Misses 724 724
Partials 186 186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
esp32float formatting path enabled via--undefined=_printf_float(target ldflags)libmintegration from minimal subset to full command-derived groupslibm-default-xtensa.alibm-fbuiltin_fno_math_errno-xtensa.alibm-machine_xtensa_d_libm-xtensa.alibmcompile includes for CI fresh envnewlib/libc/machine/xtensa/include(xtensa/config/core-isa.h)newlib/libc/xtensa(prefer xtensasys/fenv.h)newlib/libm/math/ef_acosh.cHow The libm List Was Generated (Reproducible)
The xtensa
libmfile list and flags are derived from actualnewlibbuild commands, not guessed manually.bearwrapper mode:arguments):group + file):Why
ef_acosh.cWas Added379entries.bearcapture showed one additional default-file compile unit:libm/math/ef_acosh.c.380.Validation
go test ./internal/crosscompile/compile/libcgo build -o /tmp/llgo-local ./cmd/llgo && cd _demo/embed && /tmp/llgo-local run -a -target=esp32 -emulator ./esp32/float-1685