fix(esp32c3): correct -march to rv32imc to prevent illegal instruction crashes#1443
fix(esp32c3): correct -march to rv32imc to prevent illegal instruction crashes#1443xushiwei merged 3 commits intogoplus:mainfrom
Conversation
Summary of ChangesHello @luoliwoshang, 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 a critical issue causing illegal instruction crashes on the ESP32-C3 microcontroller by precisely configuring its RISC-V instruction set architecture. It introduces a dedicated target profile for ESP RISC-V, updates the Newlib dependency, and integrates essential bare-metal support for USB Serial JTAG output, significantly enhancing the stability and usability of TinyGo on ESP32-C3 devices. 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces bare-metal USB Serial JTAG support for the ESP32-C3, enabling stdout/stderr output. Key changes include adding a new C file (targets/device/esp/esp32c3.c) with ESP32-C3 USB Serial JTAG register definitions, low-level write functions, and a _write syscall implementation that handles newline to CRLF conversion and non-blocking character writing. The esp32c3.json target configuration is updated to inherit from a new riscv32-esp base, include the new C file, and define the USB Serial JTAG memory address in esp32c3.memory.ld. Additionally, the riscv32-esp target's llvm-target and cflags are adjusted to riscv32-esp-elf and -march=rv32imc respectively, reflecting the ESP32-C3's lack of the 'A' (Atomic) extension. This architectural distinction is also incorporated into internal/crosscompile/crosscompile.go and internal/crosscompile/compile/rtlib/compiler_rt.go to conditionally include atomic.c for non-ESP RISC-V32 targets. The newlib-esp32 library configuration is also updated to patch4. Review comments point out that the new _write function in esp32c3.c incorrectly reports all bytes as written despite its non-blocking nature potentially dropping characters, and it lacks a check for valid file descriptors (1 and 2), which could lead to silent data loss or unexpected behavior.
Code Review SummaryThis PR successfully fixes the ESP32-C3 illegal instruction crash through a well-architected solution. The approach of creating a dedicated Key strengths:
Noteworthy issues identified:
See inline comments for specific recommendations. |
087d780 to
65d17bc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1443 +/- ##
==========================================
+ Coverage 90.54% 90.58% +0.03%
==========================================
Files 43 43
Lines 11423 11429 +6
==========================================
+ Hits 10343 10353 +10
+ Misses 917 914 -3
+ Partials 163 162 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
65d17bc to
ba5ce7d
Compare
…nstructions Problem: - ESP32-C3 only supports RV32IMC (no A extension for atomic operations) - Using -march=rv32imac generates unsupported atomic instructions - Including atomic.c causes compilation failure due to missing hardware support Solution: - Create riscv32-esp.json as base config for ESP RISC-V chips - Use llvm-target "riscv32-esp-elf" to identify ESP targets - Change -march from rv32imac to rv32imc for ESP targets - Conditionally exclude atomic.c for ESP targets in compiler-rt This fix applies to all ESP32-C3 variants through inheritance: - esp32c3 - esp32c3-supermini - m5stamp-c3 - esp32-c3-devkit-rust-1 - esp-c3-32s-kit - esp32c3-12f Reference: ESP-IDF uses riscv32-esp-elf as official target triple
68b9163 to
359503d
Compare
Add test cases to verify correct -march flag selection: - riscv32 (generic): expects -march=rv32imac (with A extension) - esp32c3: expects -march=rv32imc (no A extension) This covers both branches of the ESP RISC-V chip detection logic.
359503d to
305253c
Compare
Code Review SummaryThis PR correctly fixes the ESP32-C3 illegal instruction issue by using the appropriate |
- Change compiler_rt.go to use exact matching (target != "riscv32-esp-elf") instead of strings.Contains for consistency with crosscompile.go - Remove redundant -march=rv32imc from esp32c3.json since it's already inherited from riscv32-esp.json (follows DRY principles)
part of #1427
1. Illegal Instruction Crash with
println()After implementing USB Serial JTAG output:
c.Printf()works fine - Output visible through USB Serial JTAGprintln()crashes the program - Triggers illegal instruction exceptionCrash Details:
Root Cause:
-march=rv32imacenables A extension (atomic operations)C.FLD(compressed floating-point load)2. Compiler-RT atomic.c Build Failure
After fixing the
-marchflag torv32imc(removing A extension), a new issue appears:Build Error:
Cause:
atomic.crequires hardware atomic support (A extension), which ESP32-C3 doesn't have.This affects all ESP32-C3 board variants.
Solution
1. Fix
-marchFlag to Remove A ExtensionCreate
riscv32-esp.json: Base configuration for ESP RISC-V chipsriscv32-esp-elfas llvm-target (official target triple from ESP-IDF toolchain)-march=rv32imc(no A/D/F extensions)-march=rv32imcfor ESP32-C3 and all ESP RISC-V chipsUse
llvm-targetfor identification: Checkconfig.LLVMTarget == "riscv32-esp-elf"instead of hardcoded target namesResult: No more illegal instruction crashes -
println()now works correctly.2. Exclude atomic.c for ESP Targets
Conditionally exclude
atomic.c: Skip compilation for ESP targets that lack A extensionatomic.cResult: Clean compilation without atomic.c errors.
Affected Targets
This fix applies to all ESP32-C3 variants through inheritance: