From 0475ae87e9b96ea8fc7c928f4a7ce26b60ce71df Mon Sep 17 00:00:00 2001 From: Ghaith Hachem Date: Mon, 18 May 2026 13:25:42 +0200 Subject: [PATCH 1/3] feat(driver): add per-phase pipeline timing instrumentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a small RAII timer (`PhaseTimer`) that records wall-clock time for each stage of `BuildPipeline` and for every participant invocation (`pre_index`, `post_index`, `pre_annotate`, `post_annotate`). The inner `ParsedProject::index` and `IndexedProject::annotate` methods self-time, so when a participant triggers an implicit whole-project re-index or re-annotate from inside one of its hooks, the cost shows up nested under the participant that caused it. The instrumentation is gated on `PLC_TIMING=1`. With the env var unset (the default, including CI), the timers compile to a near-noop and emit nothing — no behavioural change to existing tests, no diagnostic drift, no stdout/stderr noise. `PhaseTimer::new` takes a `&'static str` for static labels; `PhaseTimer::new_with(|| format!(...))` defers label allocation for dynamic labels so the instrumentation doesn't perturb the disabled-state baseline. `PipelineParticipant` and `PipelineParticipantMut` gain a `name()` method, used as the timer label. The default returns the implementing type's short name (module path and generics stripped); participants can override if needed. Documented in `book/src/development/phase_timing.md`. `scripts/oscat_multi_split.py` is bundled for benchmarking against multi-unit corpora. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitignore | 5 + book/src/SUMMARY.md | 2 + book/src/development/phase_timing.md | 90 +++++++++++++++++ compiler/plc_driver/src/pipelines.rs | 74 ++++++++++---- .../plc_driver/src/pipelines/participant.rs | 10 ++ compiler/plc_driver/src/pipelines/timing.rs | 96 +++++++++++++++++++ scripts/oscat_multi_split.py | 94 ++++++++++++++++++ 7 files changed, 350 insertions(+), 21 deletions(-) create mode 100644 book/src/development/phase_timing.md create mode 100644 compiler/plc_driver/src/pipelines/timing.rs create mode 100644 scripts/oscat_multi_split.py diff --git a/.gitignore b/.gitignore index 79ef2e904c3..1106f792d48 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,8 @@ tests/lit/**/*.txt tests/lit/**/Output/ tests/lit/**/header_generator/**/*.h !tests/lit/**/header_generator/**/dependencies.plc.h + +# Local-only baseline / test corpora (not committed) +/.baseline/ +/benchmarks/local/ + diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 30cf6f43164..21621a7d406 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -25,3 +25,5 @@ - [Codegen](./arch/codegen.md) - [CFC](./cfc/cfc.md) - [Model-to-Model Conversion](./cfc/m2m.md) +- [Development]() + - [Profiling Build Phases](./development/phase_timing.md) diff --git a/book/src/development/phase_timing.md b/book/src/development/phase_timing.md new file mode 100644 index 00000000000..270292ab132 --- /dev/null +++ b/book/src/development/phase_timing.md @@ -0,0 +1,90 @@ +# Profiling Build Phases + +The compiler driver has a built-in phase timer that records wall-clock +time for each stage of `BuildPipeline` and for every participant +invocation. It is intended for ad-hoc performance work — for example, +when investigating why a particular project compiles slower than +expected, or when measuring the impact of a change to the lowering +pipeline. + +## Enabling + +Set the environment variable `PLC_TIMING=1` before invoking the +compiler: + +```sh +PLC_TIMING=1 plc --check my_project.st +``` + +When the variable is unset (or set to `0` / empty), the timers compile +to a no-op and emit nothing. There is no behavioural difference and no +test impact, so it is safe to leave the code path always present. + +## Reading the output + +Each timed scope writes one line to stderr on completion, indented by +nesting depth. Children appear *before* their parent's log line (the +parent prints when it drops, i.e. at end-of-scope), which matches the +standard flame-graph convention. + +A condensed example from compiling a small project: + +```text +[plc-timing] parse: 25.7ms +[plc-timing] pre_index (participants): 12.6ms +[plc-timing] pre_index/LoopDesugarer: 6.6ms +[plc-timing] pre_index/ControlStatementParticipant: 3.0ms +[plc-timing] ParsedProject::index: 7.4ms +[plc-timing] post_index (participants): 27.6ms +[plc-timing] post_index/PolymorphismLowerer: 11.7ms +[plc-timing] ParsedProject::index: 9.6ms +[plc-timing] post_index/RetainParticipant: 15.8ms +[plc-timing] ParsedProject::index: 5.8ms +[plc-timing] index (driver): 47.6ms +[plc-timing] annotate (driver): 615.2ms +``` + +A few things to note from this trace: + +- The outer `parse`, `index (driver)`, `annotate (driver)`, and + `generate (driver)` scopes correspond to the four top-level phases. +- Each participant invocation is timed individually with a label of the + form `/`, e.g. `post_index/PolymorphismLowerer`. +- `ParsedProject::index` and `IndexedProject::annotate` self-time, so + any participant that re-invokes them appears with a nested + `ParsedProject::index` (or `IndexedProject::annotate`) child line. + Those nested re-passes are the main thing to look for when + investigating slow builds. + +## What to look for + +The trace is most useful for spotting **redundant whole-project +re-passes**: cases where a participant mutates the AST and then re-runs +indexing or annotation against the entire project, even though only a +few units were touched. A nested `ParsedProject::index` or +`IndexedProject::annotate` under a participant hook is a visible +indicator of one of those re-passes. + +## Adding new timed scopes + +To time a new scope, construct a `PhaseTimer` and let it drop at the +end of the scope you want to measure: + +```rust +use crate::pipelines::timing::PhaseTimer; + +fn expensive_thing() { + let _t = PhaseTimer::new("expensive_thing"); + // ... work ... +} +``` + +For participant-style instrumentation, prefer wrapping the *callee* +(the inner method that does the work) rather than each call site. That +way a re-entrant call from a participant gets timed automatically, +without having to thread timer code through every place that might +call into the method. + +The timer label is the only argument and accepts any +`Into`. Use a stable, easily-greppable string — these strings +end up in the trace output and may be parsed by tooling. diff --git a/compiler/plc_driver/src/pipelines.rs b/compiler/plc_driver/src/pipelines.rs index 833978504d4..c2533d9f25c 100644 --- a/compiler/plc_driver/src/pipelines.rs +++ b/compiler/plc_driver/src/pipelines.rs @@ -58,6 +58,9 @@ use serde_json; use toml; pub mod participant; +pub mod timing; + +use timing::PhaseTimer; pub mod property; pub struct BuildPipeline { @@ -448,39 +451,65 @@ impl Pipeline for BuildPipeline { } fn parse(&mut self) -> Result { + let _t = PhaseTimer::new("parse"); let project = ParsedProject::parse(&self.context, &self.project, &mut self.diagnostician)?; Ok(project) } fn index(&mut self, project: ParsedProject) -> Result { - self.participants.iter_mut().for_each(|p| { - p.pre_index(&project); - }); - let project = self.mutable_participants.iter_mut().fold(project, |project, p| p.pre_index(project)); + let _t = PhaseTimer::new("index (driver)"); + let project = { + let _t = PhaseTimer::new("pre_index (participants)"); + self.participants.iter_mut().for_each(|p| { + let _t = PhaseTimer::new_with(|| format!("pre_index/{}", p.name())); + p.pre_index(&project); + }); + self.mutable_participants.iter_mut().fold(project, |project, p| { + let _t = PhaseTimer::new_with(|| format!("pre_index/{}", p.name())); + p.pre_index(project) + }) + }; let indexed_project = project.index(self.context.provider()); - self.participants.iter().for_each(|p| { - p.post_index(&indexed_project); - }); - let project = - self.mutable_participants.iter_mut().fold(indexed_project, |project, p| p.post_index(project)); + let project = { + let _t = PhaseTimer::new("post_index (participants)"); + self.participants.iter().for_each(|p| { + let _t = PhaseTimer::new_with(|| format!("post_index/{}", p.name())); + p.post_index(&indexed_project); + }); + self.mutable_participants.iter_mut().fold(indexed_project, |project, p| { + let _t = PhaseTimer::new_with(|| format!("post_index/{}", p.name())); + p.post_index(project) + }) + }; Ok(project) } fn annotate(&mut self, project: IndexedProject) -> Result { - self.participants.iter().for_each(|p| { - p.pre_annotate(&project); - }); - let project = - self.mutable_participants.iter_mut().fold(project, |project, p| p.pre_annotate(project)); + let _t = PhaseTimer::new("annotate (driver)"); + let project = { + let _t = PhaseTimer::new("pre_annotate (participants)"); + self.participants.iter().for_each(|p| { + let _t = PhaseTimer::new_with(|| format!("pre_annotate/{}", p.name())); + p.pre_annotate(&project); + }); + self.mutable_participants.iter_mut().fold(project, |project, p| { + let _t = PhaseTimer::new_with(|| format!("pre_annotate/{}", p.name())); + p.pre_annotate(project) + }) + }; let annotated_project = project.annotate(self.context.provider()); - self.participants.iter().for_each(|p| { - p.post_annotate(&annotated_project); - }); - let mut annotated_project = self - .mutable_participants - .iter_mut() - .fold(annotated_project, |project, p| p.post_annotate(project)); + let mut annotated_project = { + let _t = PhaseTimer::new("post_annotate (participants)"); + self.participants.iter().for_each(|p| { + let _t = PhaseTimer::new_with(|| format!("post_annotate/{}", p.name())); + p.post_annotate(&annotated_project); + }); + self.mutable_participants.iter_mut().fold(annotated_project, |project, p| { + let _t = PhaseTimer::new_with(|| format!("post_annotate/{}", p.name())); + p.post_annotate(project) + }) + }; // Collect diagnostics from participants that validated during lowering. for p in &mut self.mutable_participants { @@ -491,6 +520,7 @@ impl Pipeline for BuildPipeline { } fn generate(&mut self, _context: &CodegenContext, project: AnnotatedProject) -> Result<(), Diagnostic> { + let _t = PhaseTimer::new("generate (driver)"); self.participants.iter_mut().try_fold((), |_, participant| participant.pre_generate(&project))?; let Some(compile_options) = self.get_compile_options() else { log::debug!("No compile options provided"); @@ -686,6 +716,7 @@ impl ParsedProject { /// Creates an index out of a pased project. The index could then be used to query datatypes pub fn index(self, id_provider: IdProvider) -> IndexedProject { + let _t = PhaseTimer::new("ParsedProject::index"); let indexed_units = self .units .into_par_iter() @@ -736,6 +767,7 @@ pub struct IndexedProject { impl IndexedProject { /// Creates annotations on the project in order to facilitate codegen and validation pub fn annotate(self, mut id_provider: IdProvider) -> AnnotatedProject { + let _t = PhaseTimer::new("IndexedProject::annotate"); //Create and call the annotator let mut annotated_units = Vec::new(); let mut all_annotations = AnnotationMapImpl::default(); diff --git a/compiler/plc_driver/src/pipelines/participant.rs b/compiler/plc_driver/src/pipelines/participant.rs index 22a9764cc9d..966d3851c72 100644 --- a/compiler/plc_driver/src/pipelines/participant.rs +++ b/compiler/plc_driver/src/pipelines/participant.rs @@ -34,6 +34,11 @@ use super::{AnnotatedProject, AnnotatedUnit, GeneratedProject, IndexedProject, P /// Implementors can decide parse the Ast and project information /// to do actions like validation or logging pub trait PipelineParticipant: Sync + Send { + /// Short label for this participant, used by the phase-timing + /// instrumentation. Default returns the implementing type's name. + fn name(&self) -> &'static str { + super::timing::short_type_name(std::any::type_name::()) + } /// Implement this to access the project before it gets indexed /// This happens directly after parsing fn pre_index(&mut self, _parsed_project: &ParsedProject) {} @@ -69,6 +74,11 @@ pub trait PipelineParticipant: Sync + Send { /// If a previous step is being modified, such as the AST or index, /// the caller is responsible for calling the previous steps pub trait PipelineParticipantMut { + /// Short label for this participant, used by the phase-timing + /// instrumentation. Default returns the implementing type's name. + fn name(&self) -> &'static str { + super::timing::short_type_name(std::any::type_name::()) + } /// Implement this to access the project before it gets indexed /// This happens directly after parsing fn pre_index(&mut self, parsed_project: ParsedProject) -> ParsedProject { diff --git a/compiler/plc_driver/src/pipelines/timing.rs b/compiler/plc_driver/src/pipelines/timing.rs new file mode 100644 index 00000000000..4192547ba1e --- /dev/null +++ b/compiler/plc_driver/src/pipelines/timing.rs @@ -0,0 +1,96 @@ +//! Pipeline phase timing. +//! +//! Enabled by setting `PLC_TIMING=1` in the environment. When enabled, +//! each timed scope logs its elapsed wall-clock time to stderr on drop, +//! indented by nesting depth so re-entrant work (e.g. a participant that +//! triggers a project-wide re-index) is visible. + +use std::cell::Cell; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Once; +use std::time::Instant; + +static ENABLED: AtomicBool = AtomicBool::new(false); +static INIT: Once = Once::new(); + +thread_local! { + static DEPTH: Cell = const { Cell::new(0) }; +} + +fn enabled() -> bool { + INIT.call_once(|| { + let on = std::env::var("PLC_TIMING").map(|v| !v.is_empty() && v != "0").unwrap_or(false); + ENABLED.store(on, Ordering::Relaxed); + }); + ENABLED.load(Ordering::Relaxed) +} + +/// RAII guard that records a labelled phase timing. Logs to stderr on drop +/// when `PLC_TIMING` is enabled. No-op otherwise. +pub struct PhaseTimer { + label: String, + start: Instant, + depth: usize, + active: bool, +} + +impl PhaseTimer { + /// Constructs a timer with a label known at compile time. When timing is + /// disabled, the only work done is reading the `ENABLED` flag. + pub fn new(label: &'static str) -> Self { + Self::new_inner(active_depth(), label, enabled()) + } + + /// Constructs a timer whose label requires runtime formatting. The closure + /// is only invoked when timing is enabled — `format!`/allocation is + /// skipped on the hot path when `PLC_TIMING` is unset. + pub fn new_with(label_fn: F) -> Self + where + F: FnOnce() -> String, + { + let active = enabled(); + let label: String = if active { label_fn() } else { String::new() }; + Self::new_inner(active_depth(), label, active) + } + + fn new_inner(depth: usize, label: impl Into, active: bool) -> Self { + Self { label: label.into(), start: Instant::now(), depth, active } + } +} + +fn active_depth() -> usize { + if enabled() { + DEPTH.with(|d| { + let cur = d.get(); + d.set(cur + 1); + cur + }) + } else { + 0 + } +} + +impl Drop for PhaseTimer { + fn drop(&mut self) { + if !self.active { + return; + } + let elapsed = self.start.elapsed(); + let indent = " ".repeat(self.depth); + eprintln!("[plc-timing] {indent}{}: {:.3?}", self.label, elapsed); + DEPTH.with(|d| d.set(d.get().saturating_sub(1))); + } +} + +/// Strip the leading module path and any generic-parameter suffix from a +/// `std::any::type_name` result so the emitted label is just the type's +/// short name. Examples: +/// `foo::bar::Baz` -> `Baz` +/// `foo::Baz` -> `Baz` +pub fn short_type_name(full: &'static str) -> &'static str { + let base = match full.find('<') { + Some(idx) => &full[..idx], + None => full, + }; + base.rsplit("::").next().unwrap_or(base) +} diff --git a/scripts/oscat_multi_split.py b/scripts/oscat_multi_split.py new file mode 100644 index 00000000000..c5a7f55bd8f --- /dev/null +++ b/scripts/oscat_multi_split.py @@ -0,0 +1,94 @@ +#!/usr/bin/env python3 +"""Split a single-file `oscat.st` corpus into one POU per file. + +Each `FUNCTION` / `FUNCTION_BLOCK` / `PROGRAM` / `CLASS` / `TYPE` +declaration is emitted to its own `.st` file under +`.baseline/oscat-multi/src/`. Mirrors the "one POU per unit" style of +real PLC projects and gives `cargo xtask metrics`-style measurements a +representative multi-unit corpus. + +Run from the repo root after cloning oscat into `.baseline/oscat/`: + + git clone --depth 1 https://github.com/plc-lang/oscat .baseline/oscat + python3 scripts/oscat_multi_split.py + +The output directory is gitignored. Pass `--out` to override. +""" + +import argparse +import os +import re +import shutil + + +END_KINDS = {"FUNCTION_BLOCK", "FUNCTION", "PROGRAM", "CLASS", "TYPE"} +START = re.compile(r"^(FUNCTION_BLOCK|FUNCTION|PROGRAM|CLASS|TYPE)\s+(\S+)") +END = re.compile(r"^END_(FUNCTION_BLOCK|FUNCTION|PROGRAM|CLASS|TYPE|VAR_GLOBAL|VAR_CONFIG)\b") + + +def sanitize(name: str) -> str: + return re.sub(r"[^A-Za-z0-9_]", "_", name) + + +def split(src_path: str, out_dir: str) -> int: + with open(src_path) as f: + lines = f.read().split("\n") + if os.path.isdir(out_dir): + shutil.rmtree(out_dir) + os.makedirs(out_dir, exist_ok=True) + + preamble: list[str] = [] + i = 0 + while i < len(lines) and not START.match(lines[i]): + preamble.append(lines[i]) + i += 1 + if preamble: + with open(os.path.join(out_dir, "_preamble.st"), "w") as f: + f.write("\n".join(preamble) + "\n") + + counts: dict[str, int] = {} + cur_name: str | None = None + buf: list[str] = [] + + while i < len(lines): + line = lines[i] + m = START.match(line) + if m and cur_name is None: + cur_name = m.group(2) + buf = [line] + elif cur_name is not None: + buf.append(line) + m_end = END.match(line) + if m_end and m_end.group(1) in END_KINDS: + sane = sanitize(cur_name) + counts[sane] = counts.get(sane, 0) + 1 + suffix = f"_{counts[sane]}" if counts[sane] > 1 else "" + fname = f"{sane}{suffix}.st" + with open(os.path.join(out_dir, fname), "w") as f: + f.write("\n".join(buf) + "\n") + cur_name = None + buf = [] + i += 1 + + if buf: + with open(os.path.join(out_dir, "_tail.st"), "w") as f: + f.write("\n".join(buf) + "\n") + + return len([f for f in os.listdir(out_dir) if f.endswith(".st")]) + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--src", default=".baseline/oscat/oscat.st", help="path to the single-file oscat source" + ) + parser.add_argument( + "--out", default=".baseline/oscat-multi/src", help="output directory for per-POU files" + ) + args = parser.parse_args() + written = split(args.src, args.out) + print(f"wrote {written} files to {args.out}") + + +if __name__ == "__main__": + main() From f41aebaaa9f0b5f0209bafc8f522a8f2c32c029f Mon Sep 17 00:00:00 2001 From: Ghaith Hachem Date: Mon, 18 May 2026 14:11:43 +0200 Subject: [PATCH 2/3] feat(driver): time the link step and document build vs check traces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wraps the link call in the codegen participant's `post_generate` hook with a `PhaseTimer`, so linking shows up as its own nested line under `generate (driver)` instead of being invisible inside that block. Extends `book/src/development/phase_timing.md` with two example traces — `plc build` (full pipeline, all four top-level driver scopes visible, with `link` nested under `generate (driver)`) and `plc check` (trace truncates at `annotate (driver)` because codegen and linking never run). Co-Authored-By: Claude Opus 4.7 (1M context) --- book/src/development/phase_timing.md | 50 ++++++++++++++----- .../plc_driver/src/pipelines/participant.rs | 18 ++++--- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/book/src/development/phase_timing.md b/book/src/development/phase_timing.md index 270292ab132..c25db629dd9 100644 --- a/book/src/development/phase_timing.md +++ b/book/src/development/phase_timing.md @@ -27,7 +27,22 @@ nesting depth. Children appear *before* their parent's log line (the parent prints when it drops, i.e. at end-of-scope), which matches the standard flame-graph convention. -A condensed example from compiling a small project: +Each participant invocation is timed individually with a label of +the form `/`, e.g. +`post_index/PolymorphismLowerer`. `ParsedProject::index` and +`IndexedProject::annotate` self-time, so any participant that +re-invokes them appears with a nested `ParsedProject::index` (or +`IndexedProject::annotate`) child line. Those nested re-passes are +the main thing to look for when investigating slow builds. + +## Examples + +### `plc build` (full pipeline) + +`plc build plc.json` runs every phase end-to-end — parsing, indexing, +annotation, codegen, and linking. The trace shows all four +top-level driver scopes, with `link` nested inside +`generate (driver)`: ```text [plc-timing] parse: 25.7ms @@ -42,19 +57,30 @@ A condensed example from compiling a small project: [plc-timing] ParsedProject::index: 5.8ms [plc-timing] index (driver): 47.6ms [plc-timing] annotate (driver): 615.2ms +[plc-timing] link: 32.1ms +[plc-timing] generate (driver): 184.4ms +``` + +The default mode (`plc ` without a subcommand) follows the +same shape — same four scopes, same nesting. + +### `plc check` (front-end only) + +`plc check plc.json` (or the global `--check` flag) stops the +pipeline after annotation; codegen and linking never run, so the +trace ends at `annotate`: + +```text +[plc-timing] parse: 24.9ms +[plc-timing] pre_index (participants): 12.4ms +[plc-timing] ParsedProject::index: 7.3ms +[plc-timing] post_index (participants): 27.0ms +[plc-timing] index (driver): 47.0ms +[plc-timing] annotate (driver): 612.8ms ``` -A few things to note from this trace: - -- The outer `parse`, `index (driver)`, `annotate (driver)`, and - `generate (driver)` scopes correspond to the four top-level phases. -- Each participant invocation is timed individually with a label of the - form `/`, e.g. `post_index/PolymorphismLowerer`. -- `ParsedProject::index` and `IndexedProject::annotate` self-time, so - any participant that re-invokes them appears with a nested - `ParsedProject::index` (or `IndexedProject::annotate`) child line. - Those nested re-passes are the main thing to look for when - investigating slow builds. +This is the right mode for measuring front-end work in isolation — +the codegen and link costs are excluded. ## What to look for diff --git a/compiler/plc_driver/src/pipelines/participant.rs b/compiler/plc_driver/src/pipelines/participant.rs index 966d3851c72..5b982b73b9b 100644 --- a/compiler/plc_driver/src/pipelines/participant.rs +++ b/compiler/plc_driver/src/pipelines/participant.rs @@ -28,6 +28,7 @@ use plc_lowering::{ use project::{object::Object, project::LibraryInformation}; use source_code::SourceContainer; +use super::timing::PhaseTimer; use super::{AnnotatedProject, AnnotatedUnit, GeneratedProject, IndexedProject, ParsedProject}; /// A Build particitpant for different steps in the pipeline @@ -210,13 +211,16 @@ impl PipelineParticipant for CodegenParticipant { fn post_generate(&self) -> Result<(), Diagnostic> { let output_name = &self.compile_options.output; - let _objects = self.objects.read().expect("Failed to aquire read lock for objects").link( - &[], //Original project objects embedded in participant - self.link_options.build_location.as_deref(), - self.link_options.lib_location.as_deref(), - output_name, - self.link_options.clone(), - )?; + let _objects = { + let _t = PhaseTimer::new("link"); + self.objects.read().expect("Failed to aquire read lock for objects").link( + &[], //Original project objects embedded in participant + self.link_options.build_location.as_deref(), + self.link_options.lib_location.as_deref(), + output_name, + self.link_options.clone(), + )? + }; if let Some(lib_location) = &self.link_options.lib_location { for library in self.libraries.iter().filter(|it| it.should_copy()).map(|it| it.get_compiled_lib()) { From 614447e8844cdd9452ec7ce48bece407de6decda Mon Sep 17 00:00:00 2001 From: Ghaith Hachem Date: Wed, 27 May 2026 14:42:55 +0200 Subject: [PATCH 3/3] perf(driver): skip lowering hooks whose preconditions don't hold (#1744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > _Drafted by Claude (Opus 4.7). Reviewer judgement applies as normal._ Builds on the timing instrumentation PR. Each PR's branch is on top of the previous; review independently. ## What this does Several post-annotate lowering participants used to unconditionally re-annotate the whole project after their hook fired, regardless of whether the project actually had any work for them. Each now guards on an exact precondition against the global index. When the precondition is false the participant returns the project unchanged — no lowerer walk, no implicit re-pass. Gates: | Participant | Hook | Predicate | |---|---|---| | `PropertyLowerer` | `post_annotate` | any property declared | | `InheritanceLowerer` | `post_annotate` | any FB / class with `EXTENDS` or `IMPLEMENTS` | | `AggregateTypeLowerer` | `post_annotate` | any POU returns an aggregate (array, struct, string) | | `PolymorphismLowerer` | `post_index` | any FB, class, method, or interface | | `PolymorphismLowerer` | `post_annotate` | any method, interface, FB / class with `EXTENDS`, or user-declared `POINTER TO` / `REFERENCE TO` an FB / class | Each predicate is **exact** for the participant it gates: when it returns false, the lowerer would walk the project and emit zero changes, and the implicit re-annotate would reproduce the existing state. ## The polymorphism split The two polymorphism hooks use different predicates on purpose. - `post_index` runs the **table pass**, which adds a `__vtable` type + `__vtable` member to every FB and class. The slot is part of the FB ABI: a downstream library consumer that extends one of these FBs must see a layout-compatible base, regardless of whether the project itself dispatches polymorphically. So the table pass fires for any FB / class / method / interface, even in a pre-OOP library that declares only methodless FBs. - `post_annotate` runs the **dispatch pass**, which rewrites call sites that need vtable indirection: method calls through the vtable, body calls through `POINTER TO Base`, calls through interface variables, `SUPER^`. A pre-OOP library that uses only standalone FBs (no methods, no `EXTENDS`, no interfaces, no pointer-to-FB members) has nothing to rewrite even though its FBs ship with vtable slots. So the dispatch pass uses a strictly tighter predicate. The `POINTER TO` / `REFERENCE TO` clause is essential for library safety: a lib that declares `VAR ptr : POINTER TO FB_B; END_VAR` and calls `ptr^()` may not have any methods or inheritance in its own compilation, but a downstream consumer can retarget that pointer to `FB_B_DERIVED`. The dispatch call site must be vtable-indirected so the derived body runs. Compiler-synthesized pointers — vtable body slots (`is_function: true`) and internal `__auto_pointer_to_X` types — are excluded from the check, so oscat-shaped libraries that have no user pointers don't trip the predicate. The split means a methodless pre-OOP library without `POINTER TO` FB usage pays the table-pass cost (necessary, ABI-shaping) but skips the dispatch pass (no-op for its shape). ## Regression tests Five new lit tests guard the boundaries the dispatch gate is protecting. All use `printf` markers so the dispatched body is directly observable. **The gate skips when it should** — concrete-typed FB member calls must resolve to the static type's body, even when that type is extended elsewhere in the compilation: - **`concrete_member_call_not_dispatched/`** — single compilation unit with `FB_A` holding a concrete `FB_B` member, plus `FB_B_DERIVED EXTENDS FB_B`. `FB_A`'s `helper()` must run `FB_B`'s body. - **`concrete_member_call_not_dispatched_external/`** — two separately-compiled units. The lib is built in isolation into `libnopoly.so` with no derivation in scope; the app declares the lib's FBs as `{external}` and adds `FB_B_DERIVED`. The pre-compiled `FB_A` inside the .so must still call `FB_B`'s body. **The gate fires when it should** — a pre-OOP library that exposes FBs by pointer reference must still dispatch polymorphically, because a downstream consumer can derive and retarget: - **`pointer_to_fb_in_pre_oop_lib_dispatches/`** — lib compiled in isolation with `VAR inst_fb_b : POINTER TO FB_B; END_VAR` and `inst_fb_b^()`. App extends `FB_B` and retargets the pointer to a derived instance. The lib's `FB_A.body`, compiled with no derivation in scope, must still dispatch through the vtable so the derived body runs. - **`reference_to_fb_in_pre_oop_lib_dispatches/`** — same shape with `REFERENCE TO FB_B`. `REFERENCE TO` is encoded as a `Pointer { auto_deref: Some(_) }`, so the same check covers it. **Known gap, documented** — `VAR_IN_OUT FB` parameters are *not* currently dispatched polymorphically. This is a pre-existing limitation in the polymorphism dispatch pass itself (reproducible on a clean `master` build, unrelated to this PR), and is tracked in https://github.com/PLC-lang/rusty/issues/1743: - **`var_in_out_fb_in_pre_oop_lib_dispatches/`** — marked `XFAIL: *`, with the run.test header pointing at #1743. Removing the `XFAIL` directive once #1743 is fixed flips it into a passing regression. Included now so the gap doesn't get forgotten. The two-compilation variants (`_external/`, `pointer_to_fb`, `reference_to_fb`, `var_in_out_fb`) all use the same `extern_st_polymorphism`-style `lit.local.cfg`: the library is pre-built into a `.so` with an isolated compilation, then the app is linked against it. ## Implication Projects that don't use a given OOP feature stop paying for that feature's lowering re-pass. Pre-OOP FB libraries — the common shape for legacy PLC code — skip every post-annotate gate except `AggregateTypeLowerer` (most legacy code does have STRING-returning functions). Projects that exercise every feature see no change. The cost of each precheck is one short loop over the index with an early return on the first hit. The dominant cost on the hot path was the lowerers themselves; the gates remove that cost when it isn't earning anything. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .../plc_driver/src/pipelines/participant.rs | 151 ++++++++++++++++++ compiler/plc_driver/src/pipelines/property.rs | 7 + src/index.rs | 7 + .../plc.json | 11 ++ .../run.test | 1 + .../src/extension.st | 9 ++ .../src/lib.st | 21 +++ .../src/main.st | 23 +++ .../app.st | 35 ++++ .../lib.st | 16 ++ .../lit.local.cfg | 41 +++++ .../run.test | 3 + .../app.st | 39 +++++ .../lib.st | 17 ++ .../lit.local.cfg | 39 +++++ .../run.test | 3 + .../app.st | 28 ++++ .../lib.st | 15 ++ .../lit.local.cfg | 37 +++++ .../run.test | 3 + .../app.st | 30 ++++ .../lib.st | 17 ++ .../lit.local.cfg | 37 +++++ .../run.test | 14 ++ 24 files changed, 604 insertions(+) create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched/plc.json create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched/run.test create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched/src/extension.st create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched/src/lib.st create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched/src/main.st create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched_external/app.st create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched_external/lib.st create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched_external/lit.local.cfg create mode 100644 tests/lit/multi/concrete_member_call_not_dispatched_external/run.test create mode 100644 tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/app.st create mode 100644 tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lib.st create mode 100644 tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg create mode 100644 tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/run.test create mode 100644 tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/app.st create mode 100644 tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lib.st create mode 100644 tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg create mode 100644 tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/run.test create mode 100644 tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/app.st create mode 100644 tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lib.st create mode 100644 tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lit.local.cfg create mode 100644 tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/run.test diff --git a/compiler/plc_driver/src/pipelines/participant.rs b/compiler/plc_driver/src/pipelines/participant.rs index 5b982b73b9b..fa0a77372bf 100644 --- a/compiler/plc_driver/src/pipelines/participant.rs +++ b/compiler/plc_driver/src/pipelines/participant.rs @@ -15,8 +15,10 @@ use std::{ use ast::provider::IdProvider; use plc::{ codegen::GeneratedModule, + index::{Index, PouIndexEntry}, lowering::{calls::AggregateTypeLowerer, polymorphism::PolymorphismLowerer}, output::FormatOption, + typesystem::DataTypeInformation, ConfigFormat, OnlineChange, Target, }; use plc_diagnostics::diagnostics::Diagnostic; @@ -294,6 +296,13 @@ impl PipelineParticipantMut for InheritanceLowerer { } fn post_annotate(&mut self, annotated_project: AnnotatedProject) -> AnnotatedProject { + // Skip if the project declares no inheritance or interfaces — the + // visit would be a no-op and the implicit re-annotate would only + // recompute the same annotations. + if !project_uses_inheritance(&annotated_project.index) { + return annotated_project; + } + let AnnotatedProject { mut units, index, annotations, diagnostics } = annotated_project; self.annotations = Some(Box::new(annotations)); self.index = Some(index); @@ -315,6 +324,13 @@ impl PipelineParticipantMut for InheritanceLowerer { impl PipelineParticipantMut for AggregateTypeLowerer { fn post_annotate(&mut self, annotated_project: AnnotatedProject) -> AnnotatedProject { + // Skip if no POU has an aggregate return type — the visit would walk + // every unit and rewrite nothing, and the implicit re-index + + // re-annotate would reproduce the existing state. + if !project_has_aggregate_returns(&annotated_project.index) { + return annotated_project; + } + let AnnotatedProject { units, index, annotations, diagnostics } = annotated_project; self.index = Some(index); self.annotation = Some(Box::new(annotations)); @@ -338,6 +354,15 @@ impl PipelineParticipantMut for AggregateTypeLowerer { impl PipelineParticipantMut for PolymorphismLowerer { fn post_index(&mut self, indexed_project: IndexedProject) -> IndexedProject { + // The table pass emits a `__vtable` type and instance member for + // every FB / class — even methodless ones. The slot is part of the + // FB ABI: a downstream library consumer that extends one of these + // FBs must see a layout-compatible base. Skip only when the + // project has no FBs, classes, methods, or interfaces at all. + if !project_needs_vtables(&indexed_project.index) { + return indexed_project; + } + let IndexedProject { mut project, index, .. } = indexed_project; self.table(&index, &mut project.units); @@ -346,6 +371,18 @@ impl PipelineParticipantMut for PolymorphismLowerer { } fn post_annotate(&mut self, annotated_project: AnnotatedProject) -> AnnotatedProject { + // Dispatch lowering rewrites call sites that need vtable + // indirection: method calls / body invocations through + // `POINTER TO `, calls through interface variables, and + // `SUPER^`. None of those can exist without methods or interfaces + // declared somewhere in the project — pre-OOP libraries (FBs + // with no methods, no `EXTENDS`, no interfaces) have nothing to + // rewrite even when their FBs ship with vtable slots for + // downstream extenders. + if !project_uses_polymorphic_dispatch(&annotated_project.index) { + return annotated_project; + } + let AnnotatedProject { units, index, annotations, diagnostics } = annotated_project; let mut units: Vec<_> = units.into_iter().map(|AnnotatedUnit { unit, .. }| unit).collect(); @@ -401,3 +438,117 @@ impl PipelineParticipantMut for ReferenceToReturnParticipant { ParsedProject { units } } } + +// ─── Precheck helpers ────────────────────────────────────────────────────── +// +// Several lowering participants used to unconditionally re-run a whole-project +// index or annotate after their hook fired, even when the lowerer had nothing +// to do on this project. The helpers below answer "is there any work for me?" +// from the already-built index so the participant can skip both the walk and +// the implicit re-pass when the answer is no. Each helper is an exact +// predicate: if it returns `false`, the lowerer would produce a project +// identical to its input. + +/// True if the project has any FB, class, method, or interface — i.e. any +/// type that needs a vtable slot emitted. The vtable layout is part of the +/// FB ABI: even a methodless FB ships with a `__vtable` member so that a +/// downstream library consumer extending that FB sees a layout-compatible +/// base. Used by `PolymorphismLowerer::post_index` (table pass). +fn project_needs_vtables(index: &Index) -> bool { + if index.get_interfaces().keys().next().is_some() { + return true; + } + index.get_pous().values().any(|p| { + matches!( + p, + PouIndexEntry::Class { .. } | PouIndexEntry::FunctionBlock { .. } | PouIndexEntry::Method { .. } + ) + }) +} + +/// True if the project has any construct whose call sites the dispatch pass +/// might rewrite into vtable-indirected form: +/// +/// - methods or interfaces (method-call dispatch through the vtable); +/// - any FB or class with `EXTENDS` (body-call dispatch and `SUPER^`); +/// - any `POINTER TO ` type. A pre-OOP library may declare a +/// pointer-to-FB member and call it via `ptr^()` with no `EXTENDS` or +/// methods in its own compilation. The lib has no way to know whether +/// a downstream consumer will retarget that pointer to a derived +/// instance, so the call site must be vtable-indirected — otherwise +/// the library binary bakes in a static call to the base body and the +/// derived body never runs. `REFERENCE TO X` is encoded as a +/// `Pointer { auto_deref: Some(_) }` in the type system, so it is +/// covered by the same check. +/// +/// Used by `PolymorphismLowerer::post_annotate`. +fn project_uses_polymorphic_dispatch(index: &Index) -> bool { + if index.get_interfaces().keys().next().is_some() { + return true; + } + let pou_match = index.get_pous().values().any(|p| match p { + PouIndexEntry::Method { .. } => true, + PouIndexEntry::FunctionBlock { super_class, .. } | PouIndexEntry::Class { super_class, .. } => { + super_class.is_some() + } + _ => false, + }); + if pou_match { + return true; + } + let is_pou_type = |type_name: &str| { + index + .find_effective_type_by_name(type_name) + .map(|t| { + let info = t.get_type_information(); + info.is_function_block() || info.is_class() + }) + .unwrap_or(false) + }; + index.get_types().values().any(|dt| match &dt.information { + DataTypeInformation::Pointer { inner_type_name, is_function, .. } => { + // Exclude two kinds of compiler-synthesized pointers: + // * function pointers (`is_function: true`) — vtable body + // slots, not user-declared pointers-to-FB; + // * internal `__auto_pointer_to_X` types emitted alongside + // every FB / class. + // Both of these exist in oscat-like libraries that have no + // user-declared `POINTER TO FB` at all; counting them would + // cause the dispatch pass to fire unnecessarily. + !is_function && !dt.is_internal() && is_pou_type(inner_type_name) + } + _ => false, + }) +} + +/// True if any POU's return type is aggregate (array, struct, or string), in +/// which case `AggregateTypeLowerer` needs to rewrite that POU's signature. +fn project_has_aggregate_returns(index: &Index) -> bool { + for pou in index.get_pous().values() { + let return_type = match pou { + PouIndexEntry::Function { return_type, .. } | PouIndexEntry::Method { return_type, .. } => { + return_type.as_str() + } + _ => continue, + }; + if return_type.is_empty() { + continue; + } + if index.get_effective_type_or_void_by_name(return_type).is_aggregate_type() { + return true; + } + } + false +} + +/// True if any POU declares a super-class or any interfaces, in which case +/// `InheritanceLowerer` needs to rewrite calls and walk inheritance chains. +fn project_uses_inheritance(index: &Index) -> bool { + index.get_pous().values().any(|p| match p { + PouIndexEntry::FunctionBlock { super_class, interfaces, .. } + | PouIndexEntry::Class { super_class, interfaces, .. } => { + super_class.is_some() || !interfaces.is_empty() + } + _ => false, + }) +} diff --git a/compiler/plc_driver/src/pipelines/property.rs b/compiler/plc_driver/src/pipelines/property.rs index d940dd91d42..19ef8d9d93a 100644 --- a/compiler/plc_driver/src/pipelines/property.rs +++ b/compiler/plc_driver/src/pipelines/property.rs @@ -19,6 +19,13 @@ impl PipelineParticipantMut for PropertyLowerer { } fn post_annotate(&mut self, project: AnnotatedProject) -> AnnotatedProject { + // Skip the rewrite pass + implicit re-annotate when the project + // declares no properties at all. Exact predicate: nothing to rewrite + // means re-annotation would reproduce the existing state. + if !project.index.has_any_properties() { + return project; + } + let AnnotatedProject { mut units, index, annotations, diagnostics } = project; self.annotations = Some(annotations); diff --git a/src/index.rs b/src/index.rs index ee772f10631..b8c1656d304 100644 --- a/src/index.rs +++ b/src/index.rs @@ -1660,6 +1660,13 @@ impl Index { self.properties.get_all(pou_name).unwrap_or(&vec![]).to_vec() } + /// True if any POU in the index declares one or more properties. Cheap; + /// short-circuits at the first hit. Used by `PropertyLowerer` to skip + /// its post-annotate hook on projects that don't use properties. + pub fn has_any_properties(&self) -> bool { + self.properties.keys().next().is_some() + } + /// return the `VariableIndexEntry` associated with the given fully qualified name using `.` as /// a delimiter. (e.g. "PLC_PRG.x", or "MyClass.MyMethod.x") pub fn find_fully_qualified_variable(&self, fully_qualified_name: &str) -> Option<&VariableIndexEntry> { diff --git a/tests/lit/multi/concrete_member_call_not_dispatched/plc.json b/tests/lit/multi/concrete_member_call_not_dispatched/plc.json new file mode 100644 index 00000000000..656440727c5 --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched/plc.json @@ -0,0 +1,11 @@ +{ + "name" : "ConcreteMemberCallNotDispatched", + "files" : [ + "src/lib.st", + "src/extension.st", + "src/main.st", + "../../util/printf.pli" + ], + "compile_type" : "Static", + "output" : "app" +} diff --git a/tests/lit/multi/concrete_member_call_not_dispatched/run.test b/tests/lit/multi/concrete_member_call_not_dispatched/run.test new file mode 100644 index 00000000000..4065b970fca --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched/run.test @@ -0,0 +1 @@ +RUN: %COMPILE build plc.json && %RUN | %CHECK %S/src/main.st diff --git a/tests/lit/multi/concrete_member_call_not_dispatched/src/extension.st b/tests/lit/multi/concrete_member_call_not_dispatched/src/extension.st new file mode 100644 index 00000000000..509b09bfb17 --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched/src/extension.st @@ -0,0 +1,9 @@ +// New code that extends FB_B from the legacy lib. The mere presence of +// `EXTENDS` in the project causes the polymorphism dispatch pass to fire +// (vtables-may-be-needed). The pass must still leave FB_A's concrete +// `helper()` call alone — the call site uses a static-typed instance, not +// a `POINTER TO FB_B`, so it does not need vtable indirection. + +FUNCTION_BLOCK FB_B_DERIVED EXTENDS FB_B + counter := counter + 9000; +END_FUNCTION_BLOCK diff --git a/tests/lit/multi/concrete_member_call_not_dispatched/src/lib.st b/tests/lit/multi/concrete_member_call_not_dispatched/src/lib.st new file mode 100644 index 00000000000..c9f0695bebd --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched/src/lib.st @@ -0,0 +1,21 @@ +// Old-style library: two FBs, no methods, no inheritance. FB_A holds a +// concrete `FB_B` member and invokes it via a plain `helper()` call (no +// pointer, no interface, no SUPER). This is the call we want to keep +// non-polymorphic — the static type of `helper` is `FB_B` and the body +// that runs must be FB_B__body, even if a third unit in the same +// compilation extends FB_B. + +VAR_GLOBAL + counter : DINT := 0; +END_VAR + +FUNCTION_BLOCK FB_B + counter := counter + 100; +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_A +VAR + helper : FB_B; +END_VAR + helper(); +END_FUNCTION_BLOCK diff --git a/tests/lit/multi/concrete_member_call_not_dispatched/src/main.st b/tests/lit/multi/concrete_member_call_not_dispatched/src/main.st new file mode 100644 index 00000000000..81f9da0551d --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched/src/main.st @@ -0,0 +1,23 @@ +FUNCTION main : DINT +VAR + a : FB_A; + derived : FB_B_DERIVED; +END_VAR + +counter := 0; + +// Invokes FB_A's body, which in turn calls `helper()` on a member of +// static type `FB_B`. Must run FB_B's body (+100), NOT FB_B_DERIVED's +// (+9000), regardless of whether the dispatch pass walked this AST. +a(); + +// CHECK: counter after a() = 100 +printf('counter after a() = %d$N', counter); + +// Sanity check: FB_B_DERIVED's body really does add 9000 when invoked +// directly on a `FB_B_DERIVED` instance. +derived(); + +// CHECK: counter after derived() = 9100 +printf('counter after derived() = %d$N', counter); +END_FUNCTION diff --git a/tests/lit/multi/concrete_member_call_not_dispatched_external/app.st b/tests/lit/multi/concrete_member_call_not_dispatched_external/app.st new file mode 100644 index 00000000000..9cd3e6edf70 --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched_external/app.st @@ -0,0 +1,35 @@ +// Downstream application linking against libnopoly.so. Re-declares the +// shape of FB_B and FB_A as `{external}` so codegen emits a reference +// rather than a definition. Adds a derived FB_B_DERIVED EXTENDS FB_B. +// +// The mere presence of `EXTENDS` here causes the polymorphism dispatch +// pass to fire while compiling *this* unit. The library has already been +// compiled in isolation — FB_A's `helper()` call inside it is already +// emitted as a direct call to FB_B__body. Running main() must print +// FB_B's body output, NOT FB_B_DERIVED's, even though both types exist +// in the final binary. + +{external} +FUNCTION_BLOCK FB_B +END_FUNCTION_BLOCK + +{external} +FUNCTION_BLOCK FB_A +VAR + helper : FB_B; +END_VAR +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_B_DERIVED EXTENDS FB_B + printf('FB_B_DERIVED body$N'); +END_FUNCTION_BLOCK + +FUNCTION main +VAR + a : FB_A; + derived : FB_B_DERIVED; +END_VAR + +a(); +derived(); +END_FUNCTION diff --git a/tests/lit/multi/concrete_member_call_not_dispatched_external/lib.st b/tests/lit/multi/concrete_member_call_not_dispatched_external/lib.st new file mode 100644 index 00000000000..df33e02dea5 --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched_external/lib.st @@ -0,0 +1,16 @@ +// Old-style library compiled in isolation. Two FBs, no methods, no +// inheritance, no interfaces. FB_A holds a concrete `FB_B` member and +// invokes it via a plain `helper()` call. The compiled .so must call +// FB_B's body directly — at lib-compile time there is no derivation in +// scope, and the call is statically resolvable to FB_B__body. + +FUNCTION_BLOCK FB_B + printf('FB_B body$N'); +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_A +VAR + helper : FB_B; +END_VAR + helper(); +END_FUNCTION_BLOCK diff --git a/tests/lit/multi/concrete_member_call_not_dispatched_external/lit.local.cfg b/tests/lit/multi/concrete_member_call_not_dispatched_external/lit.local.cfg new file mode 100644 index 00000000000..29eeae9a081 --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched_external/lit.local.cfg @@ -0,0 +1,41 @@ +import os.path +import subprocess + +stdlibLocation = lit_config.params["LIB"] +compilerLocation = lit_config.params["COMPILER"] + +test_dir = os.path.dirname(__file__) +source_path = os.path.abspath(test_dir) +rustyRootDirectory = os.path.abspath(os.path.join(test_dir, "..", "..", "..", "..")) + +tmp_lib_path = "/tmp" +tmp_lib_file = f"{tmp_lib_path}/libnopoly.so" + +# Compile lib.st into a shared library — old-style FBs, no methods, no +# inheritance, no interfaces. The polymorphism dispatch pass should skip +# this compilation entirely, leaving FB_A's `helper()` as a direct call +# to FB_B__body in the emitted .so. +try: + lib_compile = f"{compilerLocation} --shared -o {tmp_lib_file}" + lib_compile = f"{lib_compile} -liec61131std -L{stdlibLocation}/lib -i \"{stdlibLocation}/include/*.st\"" + lib_compile = f"{lib_compile} -i \"{rustyRootDirectory}/tests/lit/util/*.pli\"" + lib_compile = f"{lib_compile} --linker=cc" + lib_compile = f"{lib_compile} {source_path}/lib.st" + lit_config.note(f"Running: {lib_compile}") + subprocess.run(lib_compile, shell=True, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) +except subprocess.CalledProcessError as e: + lit_config.error(f"Failed to compile lib.st: {e.stderr.decode()}") + raise + +compile = f'{compilerLocation}' +compile = f'{compile} -o /tmp/%basename_t.out' +compile = f'{compile} -liec61131std -L{stdlibLocation}/lib -i "{stdlibLocation}/include/*.st"' +compile = f'{compile} -i "{rustyRootDirectory}/tests/lit/util/*.pli"' +compile = f'{compile} -L{tmp_lib_path} -lnopoly' +compile = f'{compile} --linker=cc' + +run_cmd = f'LD_LIBRARY_PATH="{stdlibLocation}/lib:{tmp_lib_path}" /tmp/%basename_t.out' + +config.substitutions = [s for s in config.substitutions if s[0] not in ['%COMPILE', '%RUN']] +config.substitutions.append(('%COMPILE', f'{compile}')) +config.substitutions.append(('%RUN', f'{run_cmd}')) diff --git a/tests/lit/multi/concrete_member_call_not_dispatched_external/run.test b/tests/lit/multi/concrete_member_call_not_dispatched_external/run.test new file mode 100644 index 00000000000..ded683b4308 --- /dev/null +++ b/tests/lit/multi/concrete_member_call_not_dispatched_external/run.test @@ -0,0 +1,3 @@ +RUN: %COMPILE %S/app.st && %RUN | %CHECK %s +CHECK: FB_B body +CHECK: FB_B_DERIVED body diff --git a/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/app.st b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/app.st new file mode 100644 index 00000000000..097a2c66dea --- /dev/null +++ b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/app.st @@ -0,0 +1,39 @@ +// Downstream application: extends FB_B and assigns a derived instance +// to FB_A's pointer member, then calls FB_A. The body of FB_A is +// already in the linked `libdispatchptr.so` — if the lib's compilation +// resolved `inst_fb_b^()` statically as a direct call to FB_B's body, +// the derived instance's body never runs. + +{external} +FUNCTION_BLOCK FB_B +END_FUNCTION_BLOCK + +{external} +FUNCTION_BLOCK FB_A +VAR + inst_fb_b : POINTER TO FB_B; +END_VAR +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_B_DERIVED EXTENDS FB_B + printf('FB_B_DERIVED body$N'); +END_FUNCTION_BLOCK + +FUNCTION main +VAR + a : FB_A; + base : FB_B; + derived : FB_B_DERIVED; +END_VAR + +// First call: pointer targets a concrete FB_B. The lib's FB_A.body +// invokes `inst_fb_b^()` — vtable lookup yields FB_B's body. +a.inst_fb_b := ADR(base); +a(); + +// Second call: pointer retargeted to a FB_B_DERIVED instance. The +// lib's FB_A.body, compiled in isolation, must dispatch through the +// vtable so that the derived body runs. +a.inst_fb_b := ADR(derived); +a(); +END_FUNCTION diff --git a/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lib.st b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lib.st new file mode 100644 index 00000000000..e76b70d527b --- /dev/null +++ b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lib.st @@ -0,0 +1,17 @@ +// Old-style library: FB_A holds a `POINTER TO FB_B` and invokes the +// pointee via `inst_fb_b^()`. The library compiles in isolation — at +// this point there is no derivation of FB_B in scope, no methods, no +// interfaces. The dispatch site MUST nonetheless go through the +// vtable, because a downstream consumer may extend FB_B and retarget +// the pointer. + +FUNCTION_BLOCK FB_B + printf('FB_B body$N'); +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_A +VAR + inst_fb_b : POINTER TO FB_B; +END_VAR + inst_fb_b^(); +END_FUNCTION_BLOCK diff --git a/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg new file mode 100644 index 00000000000..686f65f341c --- /dev/null +++ b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg @@ -0,0 +1,39 @@ +import os.path +import subprocess + +stdlibLocation = lit_config.params["LIB"] +compilerLocation = lit_config.params["COMPILER"] + +test_dir = os.path.dirname(__file__) +source_path = os.path.abspath(test_dir) +rustyRootDirectory = os.path.abspath(os.path.join(test_dir, "..", "..", "..", "..")) + +tmp_lib_path = "/tmp" +tmp_lib_file = f"{tmp_lib_path}/libdispatchptr.so" + +# Compile lib.st in isolation. No methods, no interfaces, no inheritance +# in scope — only a `POINTER TO FB_B` member with a `^()` call site. +try: + lib_compile = f"{compilerLocation} --shared -o {tmp_lib_file}" + lib_compile = f"{lib_compile} -liec61131std -L{stdlibLocation}/lib -i \"{stdlibLocation}/include/*.st\"" + lib_compile = f"{lib_compile} -i \"{rustyRootDirectory}/tests/lit/util/*.pli\"" + lib_compile = f"{lib_compile} --linker=cc" + lib_compile = f"{lib_compile} {source_path}/lib.st" + lit_config.note(f"Running: {lib_compile}") + subprocess.run(lib_compile, shell=True, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) +except subprocess.CalledProcessError as e: + lit_config.error(f"Failed to compile lib.st: {e.stderr.decode()}") + raise + +compile = f'{compilerLocation}' +compile = f'{compile} -o /tmp/%basename_t.out' +compile = f'{compile} -liec61131std -L{stdlibLocation}/lib -i "{stdlibLocation}/include/*.st"' +compile = f'{compile} -i "{rustyRootDirectory}/tests/lit/util/*.pli"' +compile = f'{compile} -L{tmp_lib_path} -ldispatchptr' +compile = f'{compile} --linker=cc' + +run_cmd = f'LD_LIBRARY_PATH="{stdlibLocation}/lib:{tmp_lib_path}" /tmp/%basename_t.out' + +config.substitutions = [s for s in config.substitutions if s[0] not in ['%COMPILE', '%RUN']] +config.substitutions.append(('%COMPILE', f'{compile}')) +config.substitutions.append(('%RUN', f'{run_cmd}')) diff --git a/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/run.test b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/run.test new file mode 100644 index 00000000000..ded683b4308 --- /dev/null +++ b/tests/lit/multi/pointer_to_fb_in_pre_oop_lib_dispatches/run.test @@ -0,0 +1,3 @@ +RUN: %COMPILE %S/app.st && %RUN | %CHECK %s +CHECK: FB_B body +CHECK: FB_B_DERIVED body diff --git a/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/app.st b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/app.st new file mode 100644 index 00000000000..6c4ae3b1c57 --- /dev/null +++ b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/app.st @@ -0,0 +1,28 @@ +{external} +FUNCTION_BLOCK FB_B +END_FUNCTION_BLOCK + +{external} +FUNCTION_BLOCK FB_A +VAR + target : REFERENCE TO FB_B; +END_VAR +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_B_DERIVED EXTENDS FB_B + printf('FB_B_DERIVED body$N'); +END_FUNCTION_BLOCK + +FUNCTION main +VAR + a : FB_A; + base : FB_B; + derived : FB_B_DERIVED; +END_VAR + +a.target REF= base; +a(); + +a.target REF= derived; +a(); +END_FUNCTION diff --git a/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lib.st b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lib.st new file mode 100644 index 00000000000..720941afab7 --- /dev/null +++ b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lib.st @@ -0,0 +1,15 @@ +// Old-style library using REFERENCE TO (auto-dereferencing pointer) +// instead of POINTER TO. Same library-safety property: the call site +// must dispatch through the vtable so a derived target supplied +// downstream gets its own body invoked. + +FUNCTION_BLOCK FB_B + printf('FB_B body$N'); +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_A +VAR + target : REFERENCE TO FB_B; +END_VAR + target(); +END_FUNCTION_BLOCK diff --git a/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg new file mode 100644 index 00000000000..02dc332a786 --- /dev/null +++ b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/lit.local.cfg @@ -0,0 +1,37 @@ +import os.path +import subprocess + +stdlibLocation = lit_config.params["LIB"] +compilerLocation = lit_config.params["COMPILER"] + +test_dir = os.path.dirname(__file__) +source_path = os.path.abspath(test_dir) +rustyRootDirectory = os.path.abspath(os.path.join(test_dir, "..", "..", "..", "..")) + +tmp_lib_path = "/tmp" +tmp_lib_file = f"{tmp_lib_path}/libdispatchref.so" + +try: + lib_compile = f"{compilerLocation} --shared -o {tmp_lib_file}" + lib_compile = f"{lib_compile} -liec61131std -L{stdlibLocation}/lib -i \"{stdlibLocation}/include/*.st\"" + lib_compile = f"{lib_compile} -i \"{rustyRootDirectory}/tests/lit/util/*.pli\"" + lib_compile = f"{lib_compile} --linker=cc" + lib_compile = f"{lib_compile} {source_path}/lib.st" + lit_config.note(f"Running: {lib_compile}") + subprocess.run(lib_compile, shell=True, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) +except subprocess.CalledProcessError as e: + lit_config.error(f"Failed to compile lib.st: {e.stderr.decode()}") + raise + +compile = f'{compilerLocation}' +compile = f'{compile} -o /tmp/%basename_t.out' +compile = f'{compile} -liec61131std -L{stdlibLocation}/lib -i "{stdlibLocation}/include/*.st"' +compile = f'{compile} -i "{rustyRootDirectory}/tests/lit/util/*.pli"' +compile = f'{compile} -L{tmp_lib_path} -ldispatchref' +compile = f'{compile} --linker=cc' + +run_cmd = f'LD_LIBRARY_PATH="{stdlibLocation}/lib:{tmp_lib_path}" /tmp/%basename_t.out' + +config.substitutions = [s for s in config.substitutions if s[0] not in ['%COMPILE', '%RUN']] +config.substitutions.append(('%COMPILE', f'{compile}')) +config.substitutions.append(('%RUN', f'{run_cmd}')) diff --git a/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/run.test b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/run.test new file mode 100644 index 00000000000..ded683b4308 --- /dev/null +++ b/tests/lit/multi/reference_to_fb_in_pre_oop_lib_dispatches/run.test @@ -0,0 +1,3 @@ +RUN: %COMPILE %S/app.st && %RUN | %CHECK %s +CHECK: FB_B body +CHECK: FB_B_DERIVED body diff --git a/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/app.st b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/app.st new file mode 100644 index 00000000000..4bab968b47b --- /dev/null +++ b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/app.st @@ -0,0 +1,30 @@ +// Downstream application: extends FB_B and passes a derived instance +// as the VAR_IN_OUT argument to FB_A. The lib's FB_A.body, compiled in +// isolation, must dispatch `target()` through the vtable so the +// derived body runs. + +{external} +FUNCTION_BLOCK FB_B +END_FUNCTION_BLOCK + +{external} +FUNCTION_BLOCK FB_A +VAR_IN_OUT + target : FB_B; +END_VAR +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_B_DERIVED EXTENDS FB_B + printf('FB_B_DERIVED body$N'); +END_FUNCTION_BLOCK + +FUNCTION main +VAR + a : FB_A; + base : FB_B; + derived : FB_B_DERIVED; +END_VAR + +a(target := base); +a(target := derived); +END_FUNCTION diff --git a/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lib.st b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lib.st new file mode 100644 index 00000000000..020d2b9dc91 --- /dev/null +++ b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lib.st @@ -0,0 +1,17 @@ +// Old-style library: FB_A takes a `VAR_IN_OUT FB_B` parameter (an +// implicit pointer in IEC semantics) and invokes the parameter via +// `target()`. No methods, no inheritance, no interfaces. Like the +// POINTER TO case, the lib has no way to know whether a downstream +// consumer will pass a derived instance — the call must be +// vtable-indirected. + +FUNCTION_BLOCK FB_B + printf('FB_B body$N'); +END_FUNCTION_BLOCK + +FUNCTION_BLOCK FB_A +VAR_IN_OUT + target : FB_B; +END_VAR + target(); +END_FUNCTION_BLOCK diff --git a/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lit.local.cfg b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lit.local.cfg new file mode 100644 index 00000000000..c61a0d33a56 --- /dev/null +++ b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/lit.local.cfg @@ -0,0 +1,37 @@ +import os.path +import subprocess + +stdlibLocation = lit_config.params["LIB"] +compilerLocation = lit_config.params["COMPILER"] + +test_dir = os.path.dirname(__file__) +source_path = os.path.abspath(test_dir) +rustyRootDirectory = os.path.abspath(os.path.join(test_dir, "..", "..", "..", "..")) + +tmp_lib_path = "/tmp" +tmp_lib_file = f"{tmp_lib_path}/libdispatchinout.so" + +try: + lib_compile = f"{compilerLocation} --shared -o {tmp_lib_file}" + lib_compile = f"{lib_compile} -liec61131std -L{stdlibLocation}/lib -i \"{stdlibLocation}/include/*.st\"" + lib_compile = f"{lib_compile} -i \"{rustyRootDirectory}/tests/lit/util/*.pli\"" + lib_compile = f"{lib_compile} --linker=cc" + lib_compile = f"{lib_compile} {source_path}/lib.st" + lit_config.note(f"Running: {lib_compile}") + subprocess.run(lib_compile, shell=True, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) +except subprocess.CalledProcessError as e: + lit_config.error(f"Failed to compile lib.st: {e.stderr.decode()}") + raise + +compile = f'{compilerLocation}' +compile = f'{compile} -o /tmp/%basename_t.out' +compile = f'{compile} -liec61131std -L{stdlibLocation}/lib -i "{stdlibLocation}/include/*.st"' +compile = f'{compile} -i "{rustyRootDirectory}/tests/lit/util/*.pli"' +compile = f'{compile} -L{tmp_lib_path} -ldispatchinout' +compile = f'{compile} --linker=cc' + +run_cmd = f'LD_LIBRARY_PATH="{stdlibLocation}/lib:{tmp_lib_path}" /tmp/%basename_t.out' + +config.substitutions = [s for s in config.substitutions if s[0] not in ['%COMPILE', '%RUN']] +config.substitutions.append(('%COMPILE', f'{compile}')) +config.substitutions.append(('%RUN', f'{run_cmd}')) diff --git a/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/run.test b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/run.test new file mode 100644 index 00000000000..0a8ae097480 --- /dev/null +++ b/tests/lit/multi/var_in_out_fb_in_pre_oop_lib_dispatches/run.test @@ -0,0 +1,14 @@ +// XFAIL: * +// +// Known limitation tracked in https://github.com/PLC-lang/rusty/issues/1743 : +// the polymorphism dispatch pass does not currently rewrite a call site +// that invokes a `VAR_IN_OUT FB` parameter via the bare-name form +// (`target()`). The call resolves statically to the parameter's +// declared base type, so a downstream consumer that passes a derived +// instance still sees the base body run. This is unrelated to the +// precheck-gate work — the same test fails on a baseline build without +// any gates. Remove the `XFAIL: *` directive once #1743 is fixed. + +RUN: %COMPILE %S/app.st && %RUN | %CHECK %s +CHECK: FB_B body +CHECK: FB_B_DERIVED body