🧹 Remove fallback hack for unknown struct types in x86_64 lowering - #281
🧹 Remove fallback hack for unknown struct types in x86_64 lowering#281undivisible wants to merge 2 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Tick the box to add this pull request to the merge queue (same as
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1f0229cfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Typ::Named(struct_name) => { | ||
| let fields = self.structs.get(struct_name).cloned().unwrap_or_else(|| { | ||
| // ponytail: unknown struct type — treat as scalar | ||
| vec![("val".into(), Typ::Int)] | ||
| }); | ||
| let fields = self.structs.get(struct_name).cloned().ok_or_else(|| { | ||
| format!( | ||
| "x86_64-lower: unknown struct type `{struct_name}` in `{}`", | ||
| self.fn_name | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
On x86_64, standard typed locals emitted by polyglot fronts now fail lowering: for example, the Zig front represents var value: i32 = 1 as Stmt::Let(..., Some(Typ::Named("i32")), ...), and normalize_module does not normalize annotations inside function bodies. This branch therefore reports unknown struct type i32 instead of allocating a scalar slot. Match on typ.canonical() (while retaining the error for genuinely unknown named structs) so Zig/V and other named primitive aliases remain compilable.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
f1f0229 to
ce782e1
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_967ea0a3-79c6-435c-b87a-a76947650db0) |
PR #281 — cleaned for review; not merge-ready under CoS bar.
ce782e1 to
d5aefc6
Compare
|
HOLD for Max/CoS: x86_64 unknown-struct lowering behavior change. Rebased/stripped; await CoS OK. |
|
HOLD for Max/CoS: turns unknown-struct x86_64 fallback into a hard error (behavior change). Clean after strip, but not a tiny clearly-correct merge. |
|
HOLD for Max — x86_64 unknown-struct behavior change (error vs scalar fallback); stripped drive-bys. |
PR #281 — cleaned for review; not merge-ready under CoS bar.
d5aefc6 to
c015a16
Compare
1 participant
🎯 What: Replace a temporary workaround for unknown struct types in
x86_64_lower.rswith proper error handling.💡 Why: To prevent silent generation of broken code when lowering invalid or missing struct types.
✅ Verification: Verified by running local
cargo teston theinaugurationcrate and full project tests.✨ Result: The compiler now accurately reports an error instead of using a dummy layout for unknown structs.
PR created automatically by Jules for task 11639415770610817996 started by @undivisible
Note
Low Risk
Single-path behavior change in the native emitter: invalid IR now errors instead of compiling; no runtime logic change for well-typed modules.
Overview
x86_64 local allocation for
Typ::Namedno longer fabricates a dummy single-field layout when a struct name is missing from the module’s struct table.Instead,
alloc_localfails lowering withx86_64-lower: unknown struct type \{name}` in `{fn_name}``, matching the intent of other backends (e.g. thumb lowering) and avoiding silently wrong stack slots and machine code.Reviewed by Cursor Bugbot for commit ce782e1. Configure here.