[WIP] Embedded device GC #1286

MeteorsLiu · 2025-09-09T10:35:31Z

No description provided.

niupilot · 2025-09-09T10:35:34Z

PR Review Complete ✅

I've conducted a comprehensive review of PR #1286 focusing on code quality, potential bugs, improvements, architecture decisions, and documentation consistency.

Todo List:

Analyze the scope and purpose of the ESP libc runtime changes
Review core architecture changes in crosscompile module
Examine runtime modifications and new baremetal support
Check for potential bugs and security issues
Evaluate code quality and best practices
Assess documentation consistency
Provide specific feedback and inline comments
Submit comprehensive review feedback

Summary

This PR introduces significant enhancements for ESP32 baremetal runtime support, including comprehensive libc integration and cross-compilation improvements. The changes are well-structured and follow good software engineering practices.

Architecture Assessment ⭐⭐⭐⭐⭐

Excellent architectural decisions:

Clean separation of concerns with //go:build tags for baremetal vs default environments
Modular design - libc configurations are properly encapsulated
Platform abstraction - runtime behavior appropriately differs between environments
Comprehensive ESP32 support - covers both RISC-V and Xtensa architectures

Code Quality Review

✅ Strengths

Robust Build Tag Usage (runtime/internal/clite/stdio_baremetal.go:1, runtime/internal/runtime/z_baremetal.go:1)
- Proper use of //go:build baremetal tags for platform-specific code
- Clean separation between baremetal and default implementations
Comprehensive Libc Integration (internal/crosscompile/compile/libc/newlibesp.go)
- Extensive file lists with proper include paths
- Separate configurations for RISC-V and Xtensa architectures
- Well-organized compile groups (crt0, gloss, libc)
Safe Error Handling (runtime/internal/runtime/z_baremetal.go:14-19)
- Graceful degradation in baremetal environment where debug info isn't available
- Appropriate fatal error handling
Thread Safety Considerations (runtime/internal/lib/runtime/maxprocs_baremetal.go:5)
- Correctly returns 1 for single-core baremetal environments

⚠️ Areas for Improvement

File Duplication in Build Configuration (internal/crosscompile/compile/libc/newlibesp.go:260-292)
```
// Lines 227-259 and 260-292 contain duplicate file entries
filepath.Join(baseDir, "libgloss", "riscv", "sys_access.c"),
// ... repeated again
```
Recommendation: Extract common file lists into shared variables to eliminate duplication and reduce maintenance burden.
TODO Comments Need Attention (internal/crosscompile/compile/libc/newlibesp.go:60, 1061, 1318)
```
// TODO(MeteorsLiu): support riscv
```
Recommendation: Either implement RISC-V support or create GitHub issues to track these items.
Magic Numbers (internal/crosscompile/compile/libc/newlibesp.go:897, 1883)
```
// filepath.Join(libcDir, "posix", "fnmatch.c"),
```
Recommendation: Add comments explaining why certain files are commented out.
Long Function Bodies (internal/crosscompile/compile/libc/newlibesp.go:1096-2073)
- The getNewlibESP32ConfigXtensa function is nearly 1000 lines
  Recommendation: Break this into smaller, more manageable functions by grouping related file sets.

🔒 Security Assessment

No security concerns identified
Proper use of filepath.Join() prevents path traversal
No hardcoded credentials or sensitive information
Appropriate error handling for system calls

🐛 Potential Issues

Unused Function Parameters (runtime/internal/clite/debug/debug_baremetal.go:22)
```
func Addrinfo(addr unsafe.Pointer, info *Info) c.Int {
	panic("not implemented")
}
```
Recommendation: Consider adding _ = addr; _ = info to explicitly acknowledge unused parameters.
Panic in Production Code
- Multiple panic("not implemented") calls in baremetal debug functions
  Recommendation: Consider returning error codes instead of panicking, or add build-time warnings.

Testing & Documentation

✅ Good Practices

Comprehensive test additions in libc_test.go and rt_test.go
Test coverage for new configuration functions
Proper test structure with setup and validation

📚 Documentation

Status: Good - Functions have clear names and logical organization
Improvement: Add package-level documentation for the baremetal-specific modules

Performance Considerations

✅ Efficient: Minimal runtime overhead for baremetal environments
✅ Memory conscious: Appropriate for embedded/baremetal constraints
✅ Compile-time optimization: Build tags eliminate unused code paths

Dependencies & Compatibility

✅ Clean dependencies: No problematic external dependencies
✅ Backward compatible: Changes are additive and don't break existing functionality
✅ Version management: Proper newlib version pinning

Final Recommendation

✅ APPROVE - This PR represents solid engineering work with comprehensive ESP32 baremetal support. The architectural decisions are sound, and the implementation follows Go best practices. While there are opportunities for refinement (file deduplication, TODO resolution), these don't block the core functionality.

Priority suggestions for follow-up:

Deduplicate file lists in ESP32 configurations
Address TODO comments with concrete implementation plans
Consider breaking down the large configuration functions

The code quality is high, the testing is thorough, and the feature addition is valuable for the embedded Go ecosystem.

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

luoliwoshang · 2025-09-10T05:56:58Z

runtime/internal/tinygogc/arch_386.go

NOTE:from

https://github.com/tinygo-org/tinygo/blob/3869f76887feef6c444308e7e1531b7cac1bbd10/src/runtime/arch_386.go#L19-L22

luoliwoshang · 2025-09-10T06:19:15Z

runtime/internal/tinygogc/gc_tinygo.go

+// Sweep goes through all memory and frees unmarked memory.
+// It returns how many bytes are free in the heap after the sweep.
+func sweep() (freeBytes uintptr) {
+	freeCurrentObject := false
+	var freed uint64
+	for block := gcBlock(0); block < endBlock; block++ {
+		switch block.state() {
+		case blockStateHead:
+			// Unmarked head. Free it, including all tail blocks following it.
+			block.markFree()
+			freeCurrentObject = true
+			gcFrees++
+			freed++
+		case blockStateTail:
+			if freeCurrentObject {
+				// This is a tail object following an unmarked head.
+				// Free it now.
+				block.markFree()
+				freed++
+			}
+		case blockStateMark:
+			// This is a marked object. The next tail blocks must not be freed,
+			// but the mark bit must be removed so the next GC cycle will
+			// collect this object if it is unreferenced then.
+			block.unmark()
+			freeCurrentObject = false
+		case blockStateFree:
+			freeBytes += bytesPerBlock
+		}
+	}
+	gcFreedBlocks += freed
+	freeBytes += uintptr(freed) * bytesPerBlock
+	return
+}
+
+// dumpHeap can be used for debugging purposes. It dumps the state of each heap
+// block to standard output.
+func dumpHeap() {
+	println("heap:")
+	for block := gcBlock(0); block < endBlock; block++ {
+		switch block.state() {
+		case blockStateHead:
+			print("*")
+		case blockStateTail:
+			print("-")
+		case blockStateMark:
+			print("#")
+		default: // free
+			print("·")
+		}
+		if block%64 == 63 || block+1 == endBlock {
+			println()
+		}
+	}
+}


NOTE: from https://github.com/tinygo-org/tinygo/blob/3869f76887feef6c444308e7e1531b7cac1bbd10/src/runtime/gc_blocks.go#L676-L730

luoliwoshang · 2025-09-10T06:41:06Z

runtime/internal/tinygogc/gc_tinygo.go

+//go:linkname _heapStart _heapStart
+var _heapStart [0]byte
+
+//go:linkname _heapEnd _heapEnd
+var _heapEnd [0]byte
+
+//go:linkname _globals_start _globals_start
+var _globals_start [0]byte
+
+//go:linkname _globals_end _globals_end
+var _globals_end [0]byte


NOTE: current _heapStart _heapEnd _globals_start _globals_end is define at targets/esp32.app.elf.ld

luoliwoshang · 2025-09-10T06:50:00Z

runtime/internal/tinygogc/gc_tinygo.go

+	// Mark phase: mark all reachable objects, recursively.
+	gcMarkReachable()
+
+	finishMark()


NOTE: when baremetal && hasScheduler will have another logic around finishMark()

MeteorsLiu added 7 commits September 5, 2025 16:27

fix: symbol not found when building in baremental environment

2d1120b

revert unix

0c41807

fix: rethrow in baremental

0d02a72

fix: typo

e444c4b

fix: println

7ca8abc

fix: stdio and typo

d9a20e8

Merge branch 'main' of https://github.com/goplus/llgo into emb-runtime

b39f0aa

Merge branch 'main' of https://github.com/goplus/llgo into emb-runtime

3cc8fd1

luoliwoshang reviewed Sep 10, 2025

View reviewed changes

MeteorsLiu added 3 commits September 10, 2025 14:06

Merge branch 'main' of https://github.com/goplus/llgo into emb-runtime

0ed3a54

fix: change clite's LLGoPackage to true to execute its init()

0bbdb6c

chore: remove unused file

a1d7417

luoliwoshang reviewed Sep 10, 2025

View reviewed changes

MeteorsLiu added 3 commits September 10, 2025 17:03

test: add esp32 libc and rt test

c50395b

test: remove chinses commets

dc59333

feat: support GC

dba50fb

MeteorsLiu force-pushed the esp-libc-runtime branch from 52df58c to dba50fb Compare September 11, 2025 11:44

MeteorsLiu added 2 commits September 11, 2025 22:11

fix: wrong global area

628fa44

fix: add stack

45766e3

luoliwoshang mentioned this pull request Sep 12, 2025

task3:machine lib goplus/lib#15

Draft

MeteorsLiu added 2 commits September 12, 2025 11:14

fix: add stack

ce39f12

feat: add debug

44e5de6

MeteorsLiu mentioned this pull request Sep 15, 2025

Implement baremetal GC #1278

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Embedded device GC #1286

[WIP] Embedded device GC #1286

Uh oh!

MeteorsLiu commented Sep 9, 2025

Uh oh!

niupilot bot commented Sep 9, 2025 •

edited

Loading

Uh oh!

luoliwoshang Sep 10, 2025

Uh oh!

luoliwoshang Sep 10, 2025

Uh oh!

luoliwoshang Sep 10, 2025

Uh oh!

luoliwoshang Sep 10, 2025

Uh oh!

Uh oh!

[WIP] Embedded device GC #1286

Are you sure you want to change the base?

[WIP] Embedded device GC #1286

Uh oh!

Conversation

MeteorsLiu commented Sep 9, 2025

Uh oh!

niupilot bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✅

Todo List:

Summary

Architecture Assessment ⭐⭐⭐⭐⭐

Code Quality Review

✅ Strengths

⚠️ Areas for Improvement

🔒 Security Assessment

🐛 Potential Issues

Testing & Documentation

✅ Good Practices

📚 Documentation

Performance Considerations

Dependencies & Compatibility

Final Recommendation

Uh oh!

luoliwoshang Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

luoliwoshang Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

luoliwoshang Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

luoliwoshang Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niupilot bot commented Sep 9, 2025 •

edited

Loading