fix(cgen): fix address-of and as_cast pointer codegen - #27481
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
![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: 6045f791fb
ℹ️ 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".
|
Addressed the latest review feedback in 528f9ed:
Validation:
|
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528f9edd0b
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@medvednikov / @JalonSolov From my perspective, this PR is ready to merge. Thanks! |
|
@codex review |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8605c07c38
ℹ️ 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".
|
Fixed the current-head function-variant address-cast finding in 4bd7cfe:
Validation:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bd7cfee5b
ℹ️ 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".
| if expr_type_sym.info is ast.SumType | ||
| || (expr_type_sym.info is ast.Interface && node.expr_type != node.typ) { | ||
| return true |
There was a problem hiding this comment.
When a checked interface conversion is addressed, for example if w is ResizableWidget { p := &(w as ResizableWidget) }, this helper returns true because the source and target are different interfaces, causing PrefixExpr to omit &. However, the interface-to-interface branch of as_cast emits I_Widget_as_I_ResizableWidget(w), whose generated signature returns ResizableWidget by value and which does not use is_direct_amp; assigning that value to the inferred &ResizableWidget therefore produces incompatible C. Keep the outer address operation or materialize an address for this branch.
Useful? React with 👍 / 👎.
|
Fixed the interface-to-interface address-cast finding in 1234935:
Validation on this head:
The complete C-output suite passed in the immediately preceding fix cycle; it was not overlapped again because another session was using that runner's shared temporary directory. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12349354f9
ℹ️ 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".
| tmp_var = g.new_tmp_var() | ||
| g.writeln('${typ} ${tmp_var};') |
There was a problem hiding this comment.
When the newly covered function-type or interface-to-interface cast is returned, such as return &(handler_sum as IntHandler), this branch declares an ordinary C automatic temporary, assigns the cast to it, and then returns &tmp_var. The pointer consequently dangles as soon as the function exits, potentially causing a use-after-return; the same occurs when the pointer is stored beyond the current block. Fresh evidence beyond the prior type-mismatch reports is that the new hoisting branch fixes those mismatches by introducing stack storage without applying V's normal escape-to-heap handling.
Useful? React with 👍 / 👎.
| if node.op == .amp { | ||
| if node.right is ast.ParExpr && node.right.expr is ast.AsCast | ||
| && (node.right.expr as ast.AsCast).expr is ast.CallExpr { | ||
| if node.op == .amp && node.right is ast.ParExpr && node.right.expr is ast.AsCast { |
There was a problem hiding this comment.
When a valid cast has redundant parentheses, for example &((handler_sum as IntHandler)) or &((widget as ResizableWidget)), the parser preserves nested ParExpr nodes but this condition recognizes only one wrapper; the later direct-cast check likewise removes only one. The special temporary path is therefore skipped and C generation takes the address of the function/interface conversion rvalue, which is not an lvalue and fails to compile. Fresh evidence beyond the prior direct-cast reports is the still-unhandled nested-ParExpr form; recursively unwrapping parentheses would make it follow the corrected path.
Useful? React with 👍 / 👎.
|
Fixed the escaping-temporary and nested-parenthesis findings in 2389599:
Validation on this head:
The immediately preceding complete C-output run passed (96/8 output fixtures, 125/9 C-pattern fixtures); a duplicate isolated rerun was stopped due shared-machine slowdown. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2389599410
ℹ️ 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".
| g.write('*') | ||
| } | ||
| if is_as_cast_heap { | ||
| g.write_heap_alloc(g.styp(as_cast_heap_type), as_cast_heap_type) |
There was a problem hiding this comment.
HEAP for fixed-array cast results
When the called sum/interface value is cast to a fixed-array variant, for example p := &(get_prefix() as [2]string), this path emits HEAP(<fixed-array typedef>, <array expression>). The HEAP macro initializes an element of (type[]){expr}, but C arrays are not assignable initializers, so GCC/Clang reject the generated C. The previous temporary path handled fixed arrays via declaration and memcpy; this heap materialization needs an equivalent fixed-array-specific allocation/copy path.
Useful? React with 👍 / 👎.
|
Fixed the addressed fixed-array cast finding in 8198a2d:
Validation:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8198a2db35
ℹ️ 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".
| } | ||
|
|
||
| fn (mut g Gen) as_cast_address_needs_heap(node ast.AsCast) bool { | ||
| if node.expr is ast.CallExpr { |
There was a problem hiding this comment.
When an escaping addressed cast parenthesizes its call operand, such as return &(((get_sum()) as Foo)), node.expr is an ast.ParExpr, so this check does not select heap storage. as_cast_operand_needs_tmp_eval still recognizes the enclosed call and stores its returned sum in a C automatic temporary, after which the direct-address path returns a pointer into that temporary's payload; the pointer dangles when the function returns. Fresh evidence beyond the previously reported selector/if/match forms is that this new heap helper still does not unwrap a ParExpr around a direct call.
Useful? React with 👍 / 👎.
|
Fixed the current-head lifetime finding by recursively unwrapping parenthesized cast operands before deciding heap allocation. Added the exact escaping Validation:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
3 participants
Summary
This PR fixes a C code generation issue where combining the address-of (
&) operator andas_castexpressions (specifically during smartcasts) results in incorrect pointer casting and dereferencing syntax in the generated C output.Details of Changes
In
vlib/v/gen/c/cgen.v:Gen.expr:&) operator codegen path. When the right-hand side is anAsCastexpression, we check if it evaluates to a pointer casting using the newas_cast_will_use_ptr()helper. If so, we avoid writing incorrect prefix operators (like double address-of or invalid dereference syntax).Gen.write_as_cast_call_start:g.is_amp(address-of context) to ensure the compiler generates(Type*)pointer casts rather than standard value casts when smartcasting is performed in an address-of context.Gen.as_cast_will_use_ptr:AsCastnode will result in a pointer-based representation under the hood (e.g. casting interfaces or sum types with differing base types).Checklist
v fmt -w.Tests Run
Rebuilt the V compiler and verified that all compiler and cgen tests pass:
./v -g -keepc -o ./vnew cmd/v ./vnew -silent test vlib/v/gen/c/All tests passed successfully on local machine.