Conversation
Added parser tests for retain variables Added index entries and test for retain variables in globals, programs and fbs.
A retain variable in a program is added to the global vars and a reference to the variable is added to the program If a variable contains a retain variable nested inside it is also treated as retain
During codegen, if a global variable should be retained add it to the .retain linker section
There was a problem hiding this comment.
Pull request overview
Adds support for RETAIN variable blocks by propagating retain-ness through the type system, carrying retain metadata through indexing, and emitting retained globals into a dedicated LLVM linker section. Also introduces a new lowering pipeline participant intended to move/indirect retain variables, plus associated test/snapshot updates.
Changes:
- Add
is_retaintracking to variable indexing and implement transitiveshould_retainchecks via datatype membership/aliasing/arrays. - Emit globals that
should_retaininto the LLVM section.retain. - Introduce a new lowering pass (
plc_lowering::retain) and update parser/index/codegen tests & snapshots for the new retain/constant debug output and fields.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/typesystem.rs |
Adds recursive should_retain computation on datatypes (struct/alias/array). |
src/index.rs |
Adds is_retain flag to VariableIndexEntry and should_retain() helper for codegen/lowering decisions. |
src/index/indexer/pou_indexer.rs |
Plumbs block.retain into member variables (is_retain). |
src/index/indexer/global_var_indexer.rs |
Plumbs retain into global variables (is_retain) via VarGlobalIndexer. |
src/index/indexer.rs |
Passes block.retain into the global var indexer. |
src/index/indexer/user_type_indexer.rs |
Ensures struct members are never directly marked retain (but can be contained in retained types). |
src/codegen/generators/llvm.rs |
Adds GlobalValueExt::make_retain() which sets the LLVM section to .retain. |
src/codegen/generators/variable_generator.rs |
Marks globals as retain in IR when global_variable.should_retain(...) is true. |
compiler/plc_lowering/src/retain.rs |
New lowering pass intended to move retain vars into global retain blocks and add indirection for PROGRAM retain vars. |
compiler/plc_lowering/src/lib.rs |
Exposes the new retain module. |
compiler/plc_driver/src/pipelines.rs |
Adds RetainParticipant into the driver pipeline. |
compiler/plc_driver/src/pipelines/participant.rs |
Implements PipelineParticipantMut for RetainParticipant (runs post-index, then re-indexes). |
compiler/plc_ast/src/ast.rs |
Updates Debug formatting of VariableBlock to conditionally include constant/retain fields. |
compiler/plc_source/src/source_location.rs |
Minor simplification (or_else → or). |
compiler/plc_diagnostics/src/reporter/codespan.rs |
Minor simplification (unwrap_or_else → unwrap_or). |
src/lowering/calls.rs |
Fixes assignment into boxed operator (*stmt.operator = ...). |
src/parser/tests/variable_parser_tests.rs |
Adds parsing tests for RETAIN on variable blocks. |
src/tests/adr/vla_adr.rs |
Updates ADR expectations to include is_retain: false. |
src/tests/adr/pou_adr.rs |
Updates ADR expectations to include is_retain: false. |
src/resolver/tests/resolve_expressions_tests.rs |
Updates expected variable entry to include is_retain: false. |
src/index/tests/interface_tests.rs |
Updates expected variable entries to include is_retain: false. |
src/index/tests/index_tests.rs |
Adds retain-related index tests and updates expected entries with is_retain. |
src/codegen/tests/code_gen_tests.rs |
Adds .retain section assertions for retained globals/program instances and whitespace normalization. |
src/parser/tests/snapshots/rusty__parser__tests__variable_parser_tests__date_and_time_constants_test.snap |
Snapshot update due to VariableBlock debug now showing constant: true. |
src/lowering/snapshots/*.snap |
Snapshot updates adding is_retain: false in debug outputs. |
src/index/tests/snapshots/*.snap |
Snapshot updates adding is_retain: false in debug outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
compiler/plc_lowering/src/retain.rs
Outdated
| variable.address = Some(AstFactory::create_identifier( | ||
| new_name, | ||
| variable.location.clone(), | ||
| self.ids.next_id(), | ||
| )); | ||
| block.variables.push(variable); |
There was a problem hiding this comment.
In visit_variable_block, the PROGRAM retain lowering sets variable.address to an Identifier to “point” to the new global retain variable. However, Variable.address is only interpreted as a hardware binding (HardwareBinding::from_statement only handles AstStatement::HardwareAccess), so an Identifier here will be ignored during indexing/codegen and won’t actually create indirection. Consider converting the variable to an explicit reference/pointer type and set its initializer to reference the new global (or otherwise use an existing alias/reference mechanism) instead of using address for this.
src/typesystem.rs
Outdated
| pub fn should_retain(&self, index: &Index, already_visited: BTreeSet<String>) -> bool { | ||
| // A datatype should be retained if one of its members is retain or if it is transitively containing a retain variable | ||
| self.get_type_information().should_retain(index, already_visited) | ||
| } |
There was a problem hiding this comment.
DataType::should_retain is a public API but requires callers to pass an already_visited set, which is an internal recursion detail. This makes the API easy to misuse and forces allocations at call sites. Consider providing a public should_retain(&self, index: &Index) -> bool wrapper and keep the visited-set version pub(crate)/private (and preferably pass it by &mut/& to avoid repeated cloning).
| // A datatype should be retained if one of its members is retain or if it is transitively containing a retain variable | ||
| let res = match self { | ||
| DataTypeInformation::Struct { members, .. } => { | ||
| members.iter().any(|member| member.should_retain_recursive(index, already_visited.clone())) | ||
| } |
There was a problem hiding this comment.
DataTypeInformation::should_retain clones already_visited for every struct member (already_visited.clone() inside any(...)). For large structs or deep nesting this can become unnecessarily expensive. Consider passing a mutable visited set through the recursion (or using a persistent/borrowed set) to avoid per-member cloning.
Adds support for retain sections in the code.
If a global variable is marked as retain, or has in its datatype any variable that is marked as retain, the variable will be added tot he
retainsection.Variables in PROGRAMS that are marked as retain of have variables in their datatype that are retained are converted into a pointer to a global retain variable. That variable is then in turn placed int he retain section during codegen.