feat: Add machine package validation workflow for TinyGo v0.39.0#26
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 significantly enhances the TinyGo ecosystem by introducing a comprehensive validation workflow for machine packages, specifically targeting TinyGo v0.39.0. It expands hardware support to over 90 new board targets and refactors core device packages to better integrate with the LLGO compiler, moving towards more automated device file generation. This ensures greater stability and broader compatibility for embedded Go development. Highlights
Ignored Files
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
|
luoliwoshang
force-pushed
the
adapt/tinygo-0.39.0-machine
branch
from
b2f9441 to
568ca14
Compare
![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 comprehensive machine package validation workflow for TinyGo v0.39.0, including new GitHub Actions, generation scripts, and validation scripts. It also involves a significant number of modifications across device files for ARM and AVR to ensure compatibility with the LLGO compiler. The changes are extensive but appear consistent and well-aligned with the goal of integrating with LLGO. My review focuses on improving the robustness of the new scripts and actions, and ensuring that unimplemented function stubs fail loudly to prevent silent errors.
| run: | | ||
| echo "Setting up LLGO from commit hash ${{ inputs.commit-hash }}..." | ||
| cd llgo-source | ||
| # Install LLGO from source | ||
| bash ./install.sh | ||
| # Set LLGO_ROOT to the source directory | ||
| echo "LLGO_ROOT=$PWD" >> $GITHUB_ENV | ||
| # Verify LLGO is available | ||
| llgo version || echo "Failed to get LLGO version" |
There was a problem hiding this comment.
The run script should be made more robust. It's good practice to have scripts fail immediately on any error by using set -e. Additionally, the current verification step for llgo version will not fail the workflow if llgo is not found or the command fails, which could lead to confusing errors in subsequent steps. It's better to fail early and clearly.
run: |
set -e
echo "Setting up LLGO from commit hash ${{ inputs.commit-hash }}..."
cd llgo-source
# Install LLGO from source
bash ./install.sh
# Set LLGO_ROOT to the source directory
echo "LLGO_ROOT=$PWD" >> $GITHUB_ENV
# Verify LLGO is available
llgo version| # Copy ignore list files to temporary directory | ||
| if [ -n "$ignore_list" ]; then | ||
| echo "[$name] Copying ignore list files to temporary directory: $ignore_list" | ||
| for file in $ignore_list; do |
There was a problem hiding this comment.
This loop over $ignore_list relies on word splitting and will not work correctly if any filename in the list contains spaces. While this is not an issue with the current ignore lists, it's a potential source of bugs in the future. To make the script more robust, I recommend defining ignore_list as a bash array and iterating over it properly.
For example, you could change the definitions from a string like ignore_list="file1.go file2.go" to an array: ignore_list=("file1.go" "file2.go").
Then, you can iterate over it safely like this:
for file in "${ignore_list[@]}"; do
# ...
done| func AsmFull(asm string, regs map[string]interface{}) uintptr | ||
| // | ||
| //llgo:link AsmFull llgo.asm | ||
| func AsmFull(asm string, regs map[string]interface{}) uintptr { return 0 } |
There was a problem hiding this comment.
The AsmFull function is stubbed to return 0. This could lead to silent failures if it's called, as the caller might not realize it's an unimplemented function. It would be safer to make it panic, similar to the SVCall* functions, to clearly signal that it's a TODO.
| func AsmFull(asm string, regs map[string]interface{}) uintptr { return 0 } | |
| func AsmFull(asm string, regs map[string]interface{}) uintptr { panic("TODO: AsmFull") } |
| func AsmFull(asm string, regs map[string]interface{}) uintptr | ||
| // | ||
| //llgo:link AsmFull llgo.asm | ||
| func AsmFull(asm string, regs map[string]interface{}) uintptr { return 0 } |
There was a problem hiding this comment.
The AsmFull function is stubbed to return 0. This could lead to silent failures if it's called, as the caller might not realize it's an unimplemented function. It would be safer to make it panic, similar to the SVCall* functions, to clearly signal that it's a TODO.
| func AsmFull(asm string, regs map[string]interface{}) uintptr { return 0 } | |
| func AsmFull(asm string, regs map[string]interface{}) uintptr { panic("TODO: AsmFull") } |
| func AsmFull(asm string, regs map[string]interface{}) uintptr { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
The AsmFull function is stubbed to return 0. This could lead to silent failures if it's called. It would be safer to make it panic to clearly indicate that it's not implemented.
| func AsmFull(asm string, regs map[string]interface{}) uintptr { | |
| return 0 | |
| } | |
| func AsmFull(asm string, regs map[string]interface{}) uintptr { | |
| panic("TODO: AsmFull") | |
| } |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| echo "[$device_name] Processing ${#tasks_array[@]} tasks..." | ||
| for task in "${tasks_array[@]}"; do | ||
| echo "[$device_name] Processing task: $task" | ||
| "$generator" $task "$target_dir/" |
There was a problem hiding this comment.
[SECURITY - HIGH] Potential Command Injection Vulnerability
The $generator and $task variables are executed without validation. While the current configuration is safe, this creates a code execution vector if configurations are modified.
Recommendation:
# Validate generator name
if [[ ! "$generator" =~ ^(gen-device-avr|gen-device-svd)$ ]]; then
echo "Error: Invalid generator: $generator"
return 1
fiReference: CWE-78 (OS Command Injection)
| # Clone and setup repository | ||
| echo "[$name] Setting up repository..." | ||
| rm -rf "$lib_path" | ||
| git clone "$repo" "$lib_path" |
There was a problem hiding this comment.
[SECURITY - HIGH] Path Traversal Vulnerability
The $lib_path and $target_dir variables are not validated before file operations. An attacker with repository access could specify paths like ../../.git to delete critical files.
Recommendation:
# Validate paths before use
validate_path() {
local path="$1"
if [[ "$path" =~ \.\. ]] || [[ "$path" =~ ^/ ]]; then
echo "Error: Invalid path: $path"
return 1
fi
return 0
}
validate_path "$lib_path" || return 1
validate_path "$target_dir" || return 1Reference: CWE-22 (Path Traversal)
| fmt.Println(infile) | ||
| device, err := readSVD(infile, sourceURL) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read: %w", err) |
There was a problem hiding this comment.
[PERFORMANCE - CRITICAL] Sequential File Processing
SVD files are processed sequentially in a single-threaded loop. This is significantly slower than the parallel approach used in gen-device-avr.go.
Impact: 2-5x slower than necessary for large device sets.
Recommendation: Implement parallel processing using worker goroutines similar to the AVR generator (lines 679-716 in gen-device-avr.go):
var wg sync.WaitGroup
workChan := make(chan string)
errChan := make(chan error, 1)
for i := 0; i < runtime.NumCPU(); i++ {
go func() {
for infile := range workChan {
// Process file
}
}()
}
| } | ||
|
|
||
| func main() { | ||
| indir := os.Args[1] // directory with register descriptor files (*.atdf) |
There was a problem hiding this comment.
[SECURITY - MEDIUM] Missing Input Validation
The tool directly uses command-line arguments without validation, causing panics on missing arguments.
Recommendation:
if len(os.Args) != 3 {
fmt.Fprintln(os.Stderr, "Usage: gen-device-avr <input-dir> <output-dir>")
os.Exit(1)
}
indir := filepath.Clean(os.Args[1])
outdir := filepath.Clean(os.Args[2])
// Validate no path traversal
if strings.Contains(indir, "..") || strings.Contains(outdir, "..") {
fmt.Fprintln(os.Stderr, "Error: path traversal not allowed")
os.Exit(1)
}
| for package_spec in "${DEVICE_VALIDATION_TARGETS[@]}"; do | ||
| validate_device_package "$package_spec" || true | ||
| done | ||
|
|
There was a problem hiding this comment.
[PERFORMANCE - HIGH] Sequential Validation
Validation tests run sequentially (~150 machine targets + 30 device targets), taking 15-60 minutes. With parallelization, this could be reduced to 3-10 minutes.
Recommendation: Use GNU parallel or bash background jobs:
# Parallel validation with GNU parallel
parallel -j $(nproc) validate_machine_target {} "$test_file" ::: "${MACHINE_VALIDATION_TARGETS[@]}"
# Or with bash background jobs and semaphore control
max_jobs=$(nproc)
for target in "${MACHINE_VALIDATION_TARGETS[@]}"; do
while [ $(jobs -r | wc -l) -ge $max_jobs ]; do sleep 0.1; done
validate_machine_target "$target" "$test_file" &
done
wait
| @@ -0,0 +1,727 @@ | |||
| // From tinygo v0.39.0 | |||
There was a problem hiding this comment.
[DOCUMENTATION] Missing Package Documentation
The file lacks proper package-level documentation explaining the tool's purpose and usage.
Recommendation:
// Package main provides gen-device-avr, a tool that generates Go device
// definitions from AVR microcontroller ATDF (Atmel Device File) files.
// This tool is based on TinyGo v0.39.0's device generation code.
//
// Usage:
// gen-device-avr <input-directory> <output-directory>
//
// The input directory should contain .atdf files, and generated .go files
// will be written to the output directory.
package main
| func SVCall0(num uintptr) uintptr | ||
| func SVCall0(num uintptr) uintptr { | ||
| // TODO(zzy): implement system call (SVCall) functionality for ARM Cortex-M | ||
| // TinyGo compiler implementation: tinygo/compiler/inlineasm.go emitSVCall function |
There was a problem hiding this comment.
[CODE QUALITY] Stubbed Functions with Panic
All 5 SVCall functions panic with TODO messages, making them impossible to test or use safely. The unreachable return 0 statement after panic() should be removed.
Recommendation:
func SVCall0(num uintptr) uintptr {
// TODO(zzy): implement system call (SVCall) functionality for ARM Cortex-M
// See: https://github.com/goplus/llgo/issues/XXXX
panic("SVCall0 not yet implemented - see issue #XXXX")
}Consider adding an issue reference and removing unreachable returns.
| workChan := make(chan string) | ||
| errChan := make(chan error, 1) | ||
| for i := 0; i < runtime.NumCPU(); i++ { | ||
| go func() { |
There was a problem hiding this comment.
[CODE QUALITY] Error Channel Race Condition
The error channel has a buffer size of 1, meaning only the first error is captured. If multiple workers encounter errors simultaneously, subsequent errors are silently dropped.
Recommendation:
// Buffer for all workers to prevent dropped errors
errChan := make(chan error, runtime.NumCPU())
// Or collect all errors:
var errMux sync.Mutex
var errors []error
// In worker:
if err != nil {
errMux.Lock()
errors = append(errors, err)
errMux.Unlock()
}
Comprehensive Code Review SummaryThis PR adds substantial machine package validation infrastructure for TinyGo v0.39.0. The implementation is solid overall, but requires attention to security and performance concerns before merging. Critical Issues (Must Fix)Security:
Performance:
Positive Highlights
Recommended Actions
See inline comments for specific recommendations and code examples. |
luoliwoshang
force-pushed
the
adapt/tinygo-0.39.0-machine
branch
from
639c1b0 to
f74b6a1
Compare
- Add validate-machine-packages job to workflow - Add MACHINE_VALIDATION_TARGETS with 90+ board targets - Update LLGO commit hash to 2d80951 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Sync machine package from goplus/lib PR goplus#19 - Update import paths from github.com/goplus/lib/emb to github.com/goplus/emb - Add _wrap directory for LLGO wrapper functions - Update all board and architecture specific files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add runtime/interrupt package with architecture-specific implementations - Add internal/binary, internal/bytealg, internal/itoa packages - Update import paths to github.com/goplus/emb 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
luoliwoshang
force-pushed
the
adapt/tinygo-0.39.0-machine
branch
from
c272e3e to
07f161b
Compare
luoliwoshang
force-pushed
the
adapt/tinygo-0.39.0-machine
branch
from
88caf5d to
ed381fd
Compare
luoliwoshang
force-pushed
the
adapt/tinygo-0.39.0-machine
branch
from
45c02a0 to
561ef8f
Compare
luoliwoshang
force-pushed
the
adapt/tinygo-0.39.0-machine
branch
from
561ef8f to
6dddf10
Compare
2 participants
Summary
Sync machine package and related dependencies from TinyGo v0.39.0 to provide hardware abstraction layer support for LLGO.
Synced Content
machine/runtime/interrupt/runtime/volatile/internal/binary/internal/bytealg/internal/itoa/Supported Target Platforms
Machine package supports 90+ board targets, including:
See
MACHINE_VALIDATION_TARGETSin.github/scripts/validate_embed_packages.shfor the complete list.LLGO Adaptations
1. Import Path Adaptation
TinyGo uses relative paths, which need to be changed to full module paths.
machine/ directory
"device/*""github.com/goplus/emb/device/*""runtime/*""github.com/goplus/emb/runtime/*""internal/binary""github.com/goplus/emb/internal/binary""internal/bytealg""github.com/goplus/emb/internal/bytealg""internal/itoa""github.com/goplus/emb/internal/itoa"machine/usb/ directory
"machine""github.com/goplus/emb/machine""machine/usb/*""github.com/goplus/emb/machine/usb/*""runtime/*""github.com/goplus/emb/runtime/*"2. volatile Package Adaptation
runtime/volatile/volatile.goneeds to declareLLGoPackage = "link":3. CGO to LLGO wrap C Adaptation
TinyGo uses CGO to call C functions. LLGO doesn't support CGO, so we need to use LLGO's C wrapper mechanism.
Adaptation Steps
LLGoFilesconstant in Go file to specify C file//go:linknameto link C functions: Link Go functions to C functionsgithub.com/goplus/lib/cpackageC.uint8_tc.Uint8TC.uint32_tc.Uint32TC.ulong/C.size_tc.SizeT.cfiles in_wrap/directoryRelated Files
machine_rp2040_rom.go_wrap/machine_rp2040_rom.cmachine_rp2350_rom.go_wrap/machine_rp2050_rom.c4. External Dependencies
Requires
github.com/goplus/lib/cpackage for C language interop:5. Compiler Directive Placeholders
The following functions require LLGO compiler or runtime support, currently using
panicor empty implementations as placeholders:runtime/interrupt/interrupt.go:24New()runtime/interrupt/checkpoint.go:39Checkpoint.Save()machine/machine_avr.go:149adjustMonotonicTimer()machine/machine_avr.go:153initMonotonicTimer()Validation
CI Validation
Dependencies
This PR depends on PR #25 being merged first.