Skip to content

Commit 65aeb57

Browse files
authored
fix: ensure code generation compiles (#19)
Signed-off-by: Gordon Smith <[email protected]>
1 parent 3934d24 commit 65aeb57

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

58 files changed

+5572
-3318
lines changed

.github/copilot-instructions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
- **Maintain Python equivalence**: When implementing new canonical operations or type handling, ensure the C++ behavior matches the corresponding Python code in `definitions.py`. Type names, state transitions, and error conditions should align with the reference implementation.
2828

2929
## Testing
30-
- Default build enables coverage flags on GCC/Clang; run `cmake --preset linux-ninja-Debug` followed by `cmake --build --preset linux-ninja-Debug` and then `ctest --output-on-failure` from the build tree.
30+
- Default build enables coverage flags on GCC/Clang; run `cmake --preset linux-ninja-Debug` followed by `cmake --build --preset linux-ninja-Debug` and then `ctest --output-on-failure` from the build tree. When launching tests from the repository root, add `--test-dir build` (for example `ctest --test-dir build --output-on-failure`) because calling `ctest` without a build directory just reports "No tests were found!!!".
3131
- C++ tests live in `test/main.cpp` using doctest and ICU for encoding checks; keep new tests near related sections for quick discovery.
3232
- Test coverage workflow: run tests, then `lcov --capture --directory . --output-file coverage.info`, filter with `lcov --remove`, and optionally generate HTML with `genhtml`. See README for full workflow.
3333
- **Python reference tests**: The canonical ABI test suite in `ref/component-model/design/mvp/canonical-abi/run_tests.py` exercises the same ABI rules—use it to cross-check tricky canonical ABI edge cases when changing flattening behavior. Run with `python3 run_tests.py` (requires Python 3.10+).

.github/workflows/release.yml

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,7 @@ jobs:
119119
- name: Run Tests
120120
working-directory: build
121121
run: |
122-
ctest ${{ matrix.cpack-config || '' }} -VV -E "wit-stub-generation-test"
123-
124-
- name: Upload test summary
125-
uses: actions/upload-artifact@v4
126-
with:
127-
name: ${{ matrix.os }}-test-summary
128-
path: build/test_summary.md
122+
ctest ${{ matrix.cpack-config || '' }} -VV
129123
130124
- name: Create Packages
131125
working-directory: build

.github/workflows/ubuntu.yml

Lines changed: 1 addition & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -82,66 +82,7 @@ jobs:
8282
- name: test
8383
working-directory: build
8484
run: |
85-
ctest -VV -E "wit-stub-generation-test"
86-
87-
- name: test-stubs-full (allowed to fail)
88-
working-directory: build
89-
continue-on-error: true
90-
run: |
91-
echo "Running wit-stub-generation-test (failures expected and will be reported)..."
92-
ctest -VV -R "wit-stub-generation-test" > test_output.txt 2>&1 || true
93-
cat test_output.txt
94-
../.github/scripts/summarize-test-failures.sh test_output.txt test_summary.md
95-
96-
- name: Comment PR with test summary
97-
if: github.event_name == 'pull_request'
98-
uses: actions/github-script@v7
99-
with:
100-
github-token: ${{ secrets.GITHUB_TOKEN }}
101-
script: |
102-
const fs = require('fs');
103-
const path = require('path');
104-
105-
// Check if summary file exists
106-
const summaryPath = path.join('build', 'test_summary.md');
107-
if (!fs.existsSync(summaryPath)) {
108-
console.log('Test summary file not found, skipping PR comment');
109-
return;
110-
}
111-
112-
const summary = fs.readFileSync(summaryPath, 'utf8');
113-
114-
// Find existing comment
115-
const { data: comments } = await github.rest.issues.listComments({
116-
owner: context.repo.owner,
117-
repo: context.repo.repo,
118-
issue_number: context.issue.number,
119-
});
120-
121-
const botComment = comments.find(comment =>
122-
comment.user.type === 'Bot' &&
123-
comment.body.includes('wit-stub-generation-test Results')
124-
);
125-
126-
const commentBody = `### Ubuntu Test Results\n\n${summary}\n\n---\n*Updated: ${new Date().toISOString()}*`;
127-
128-
if (botComment) {
129-
// Update existing comment
130-
await github.rest.issues.updateComment({
131-
owner: context.repo.owner,
132-
repo: context.repo.repo,
133-
comment_id: botComment.id,
134-
body: commentBody
135-
});
136-
} else {
137-
// Create new comment
138-
await github.rest.issues.createComment({
139-
owner: context.repo.owner,
140-
repo: context.repo.repo,
141-
issue_number: context.issue.number,
142-
body: commentBody
143-
});
144-
}
85+
ctest -VV
14586
14687
- name: Upload coverage reports to Codecov
14788
uses: codecov/[email protected]

.github/workflows/windows.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ jobs:
7373
- name: test
7474
working-directory: build
7575
run: |
76-
ctest -C Debug -VV -E "wit-stub-generation-test"
76+
ctest -C Debug -VV
7777
7878
- name: test-stubs-full (allowed to fail)
7979
working-directory: build

.vscode/launch.json

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,22 @@
120120
"cwd": "${workspaceFolder}",
121121
"environment": [],
122122
"preLaunchTask": "CMake: build"
123+
},
124+
{
125+
"name": "Generate & Compile WIT Test Stubs",
126+
"type": "cppdbg",
127+
"request": "launch",
128+
"program": "/usr/bin/cmake",
129+
"args": [
130+
"--build",
131+
"${workspaceFolder}/build",
132+
"--target",
133+
"test-stubs-compiled"
134+
],
135+
"stopAtEntry": false,
136+
"cwd": "${workspaceFolder}",
137+
"environment": [],
138+
"externalConsole": false
123139
}
124140
]
125141
}

docs/codegen-bugs-found.md

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
# Code Generation Bugs Found Through Comprehensive Testing
2+
3+
Generated: 2025-10-14
4+
Test: Compilation of all 199 WIT test stubs
5+
**Updated**: 2025-10-14 - After Fix #1
6+
**Result**: 182 successful (+34 from baseline), 17 failed
7+
**Success Rate**: 91.5% (was 74%)
8+
9+
## Summary
10+
11+
The comprehensive compilation test of all 199 WIT stubs successfully identified multiple categories of code generation bugs in wit-codegen. This document categorizes and prioritizes these issues.
12+
13+
**Fix #1 Status**: ✅ FIXED - Resource method naming consistency issue resolved.
14+
15+
## Bug Categories
16+
17+
### 1. Resource Method Naming Mismatch (HIGH PRIORITY) - ✅ FIXED
18+
**Affected**: Originally 12 stubs, now 10/12 fixed
19+
**Status**: **FIXED in wit-codegen** - Applied 2025-10-14
20+
21+
**Issue**: Resource methods were generated with prefixed names in headers but unprefixed names in WAMR bindings.
22+
23+
**Fix Applied**:
24+
1. Added resource name prefix to WAMR native symbol generation
25+
2. Added resource name prefix to guest function type alias generation
26+
3. Added resource name prefix to guest function wrapper generation
27+
4. Added resource type recognition in `has_unknown_type()` to prevent skipping guest function types
28+
29+
**Changes Made**:
30+
- `tools/wit-codegen/code_generator.cpp` lines 678-693: Added resource prefix for host_function symbols
31+
- `tools/wit-codegen/code_generator.cpp` lines 765-777: Added resource prefix for external package symbols
32+
- `tools/wit-codegen/code_generator.cpp` lines 1010-1024: Added resource prefix for guest function wrappers
33+
- `tools/wit-codegen/code_generator.cpp` lines 1654-1665: Added resource prefix for guest function type aliases
34+
- `tools/wit-codegen/code_generator.cpp` lines 1601-1612: Added resource type checking in has_unknown_type()
35+
36+
**Results**:
37+
- ✅ 10/12 resource stubs now compile successfully
38+
- ✅ +34 total stubs fixed (includes non-resource stubs that use resources)
39+
- ✅ Success rate improved from 74% to 91.5%
40+
41+
**Remaining resource failures** (6 stubs - different root causes):
42+
- import-and-export-resource-alias
43+
- resource-alias
44+
- resource-local-alias
45+
- resource-local-alias-borrow
46+
- resources
47+
- return-resource-from-export
48+
49+
**Example** (`import-and-export-resource-alias`):
50+
- WIT defines: `resource x { get: func() -> string; }`
51+
- Header now generates: `cmcpp::string_t x_get();`
52+
- WAMR binding now calls: `host::foo::x_get` ✓ (was `host::foo::get` ❌)
53+
- Guest function type: `using x_get_t = cmcpp::string_t();`
54+
55+
### 2. Duplicate Monostate in Variants (HIGH PRIORITY)
56+
**Affected**: 12 stubs (resource-*, import-and-export-resource*)
57+
58+
**Issue**: Resource methods are generated with prefixed names in headers but unprefixed names in WAMR bindings.
59+
60+
**Example** (`import-and-export-resource-alias`):
61+
- WIT defines: `resource x { get: func() -> string; }`
62+
- Header generates: `cmcpp::string_t x_get();`
63+
- WAMR binding tries to call: `host::foo::get` ❌ (should be `host::foo::x_get`)
64+
65+
**Affected stubs**:
66+
- import-and-export-resource
67+
- import-and-export-resource-alias
68+
- resource-alias
69+
- resource-local-alias
70+
- resource-local-alias-borrow
71+
- resource-local-alias-borrow-import
72+
- resource-own-in-other-interface
73+
- resources
74+
- resources-in-aggregates
75+
- resources-with-futures
76+
- resources-with-lists
77+
- resources-with-streams
78+
- return-resource-from-export
79+
80+
**Fix needed**: wit-codegen must consistently use resource-prefixed method names (`x_get`, `x_set`, etc.) in both headers and WAMR bindings.
81+
82+
### 2. Duplicate Monostate in Variants (HIGH PRIORITY)
83+
**Affected**: variants.wit, variants-unioning-types.wit
84+
85+
**Issue**: Variants with multiple "empty" cases generate duplicate `std::monostate` alternatives, which is invalid C++.
86+
87+
**Example** (`variants`):
88+
```cpp
89+
// Generated (INVALID):
90+
using v1 = cmcpp::variant_t<
91+
cmcpp::monostate, // First empty case
92+
cmcpp::enum_t<e1>,
93+
cmcpp::string_t,
94+
empty,
95+
cmcpp::monostate, // ❌ DUPLICATE - Second empty case
96+
uint32_t,
97+
cmcpp::float32_t
98+
>;
99+
100+
// Also affects:
101+
using no_data = cmcpp::variant_t<cmcpp::monostate, cmcpp::monostate>; // ❌ Both empty
102+
```
103+
104+
**Fix needed**: wit-codegen should generate unique wrapper types for each empty variant case, or use a numbering scheme like `monostate_0`, `monostate_1`, etc.
105+
106+
### 3. WASI Interface Function Name Mismatches (MEDIUM PRIORITY)
107+
**Affected**: 5 stubs (wasi-cli, wasi-io, wasi-http, wasi-filesystem, wasi-clocks)
108+
109+
**Issue**: Interface methods use kebab-case names in WIT but are generated with different names.
110+
111+
**Examples**:
112+
- WIT: `to-debug-string` → Generated: `error_to_debug_string` but called as `to_debug_string`
113+
- WIT: `ready`, `block` (poll methods) → Generated with prefixes or missing entirely
114+
115+
**Fix needed**: wit-codegen needs consistent snake_case conversion and proper name prefixing for interface methods.
116+
117+
### 4. Missing Guest Function Type Definitions (MEDIUM PRIORITY)
118+
**Affected**: resources-in-aggregates, resource-alias, return-resource-from-export
119+
120+
**Issue**: Guest export function types are not generated (`f_t`, `transmogriphy_t`, `return_resource_t` missing).
121+
122+
**Example** (`resources-in-aggregates`):
123+
```cpp
124+
// Header comment says:
125+
// TODO: transmogriphy - Type definitions for local types (variant/enum/record) not yet parsed
126+
127+
// But WAMR binding tries to use:
128+
guest_function<guest::aggregates::f_t>() // ❌ f_t doesn't exist
129+
```
130+
131+
**Fix needed**: wit-codegen must generate all guest function type aliases, even for resource-related functions.
132+
133+
### 5. Other Naming Issues (LOW PRIORITY)
134+
**Affected**: guest-name, issue929-only-methods, lift-lower-foreign
135+
136+
**Issue**: Edge cases in name generation for specific WIT patterns.
137+
138+
## Testing Value
139+
140+
This comprehensive test successfully identified:
141+
- **4 major bug categories** affecting 21 stubs
142+
- **Specific line numbers and examples** for each issue
143+
- **Clear reproduction cases** from the wit-bindgen test suite
144+
145+
## Recommended Fix Priority
146+
147+
1. **Resource method naming** - Affects 12 stubs, common pattern
148+
2. **Duplicate monostate** - Breaks C++ semantics, affects variant testing
149+
3. **WASI function names** - Affects important real-world interfaces
150+
4. **Missing type definitions** - Required for complete codegen
151+
5. **Edge case naming** - Lower frequency issues
152+
153+
## Next Steps
154+
155+
1. File issues in wit-codegen project with specific examples
156+
2. Add workaround detection in cmcpp for common patterns
157+
3. Continue using this test suite to validate fixes
158+
4. Expand test coverage as more WIT features are added
159+
160+
## Test Command
161+
162+
```bash
163+
# Run comprehensive compilation test:
164+
cmake --build build --target validate-root-cmake-build
165+
166+
# Check results:
167+
cat build/root_cmake_build.log | grep "error:"
168+
169+
# Count successes:
170+
grep "Built target" build/root_cmake_build.log | wc -l # 148/199
171+
```

include/cmcpp/context.hpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,14 +545,15 @@ namespace cmcpp
545545
{
546546
return 0;
547547
}
548+
auto trap_cx = make_trap_context(trap);
549+
trap_if(trap_cx, state.queue.size() > std::numeric_limits<uint32_t>::max(), "stream queue size overflow");
548550
uint32_t available = std::min<uint32_t>(max_count, static_cast<uint32_t>(state.queue.size()));
549551
if (available == 0)
550552
{
551553
return 0;
552554
}
553555
ensure_memory_range(*cx, ptr, offset + available, state.descriptor.alignment, state.descriptor.element_size);
554556
auto *dest = cx->opts.memory.data() + ptr + offset * state.descriptor.element_size;
555-
auto trap_cx = make_trap_context(trap);
556557
for (uint32_t i = 0; i < available; ++i)
557558
{
558559
const auto &bytes = state.queue.front();

include/cmcpp/flags.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ namespace cmcpp
5151
T lift_flat(const LiftLowerContext &cx, const CoreValueIter &vi)
5252
{
5353
auto i = vi.next<int32_t>();
54-
return unpack_flags_from_int<T>(static_cast<uint32_t>(i));
54+
return unpack_flags_from_int<T>(checked_int32(cx, i, "flag value exceeds 32-bit range"));
5555
}
5656
}
5757

0 commit comments

Comments
 (0)