|
FhSim
3.1.0
Marine systems simulation
|
|
|
FhSim
3.1.0
Marine systems simulation
|
| ID | 0027 |
| Class | ARCHITECTURE |
| Severity | 3 |
| Status | blocked |
| Models | — |
| Found | 2026-09-17 root-cause pass over FISH-0018, FISH-0021, FISH-0024, FISH-0026 |
| Decision needed | Which of the three remedies below to take, and whether to open anything against fhsim core. Remedy 1 is entirely in this repository; remedies 2 and 3 are changes to fhsim core and cannot be done here. |
Severity 3: this class has already put a wrong number in front of a user — the denormal on Trawl/PIDController's Out at t=0 (FISH-0021), fed straight into the door roll-control loop of examples/input/Example.xml — and the regression harness provably cannot see it (see The harness is blind to it below). Fixing the four known instances one at a time does not bound the risk, because nothing stops the next one; that is what makes this an architecture item rather than four bugs. The individual symptoms are rated 1 to 3 precisely because their visibility is decided by call ordering rather than by how wrong they are.
An FhSim outport is a member function returning const double* (fhsim/include/fhsim/simobject/PortDefs.h:18):
Its whole documented contract is the @return line above it (:16) — "Pointer to the port
output buffer (owned by the SimObject)". The framework stores the function pointer and calls it when a consumer or an observer wants the value (src/trawl/Actuator.cpp:28):
The overwhelmingly common implementation hands out the address of a member and nothing else (src/trawl/Actuator.cpp:87-90):
Every member behind such a getter is therefore part of the object's observable state at TStart, exactly like a registered state — but unlike a state it has no declared initial value anywhere. The pointer is genuinely borrowed, not copied: SerialPort caches the returned pointer (fhsim/src/engine/model/SerialPort.h:73, const double* m_cachedValue{nullptr};, assigned at fhsim/src/engine/model/SerialPort.cpp:20 and handed back at :41, with the getter at :44-47), and the observer dereferences it portSize elements deep at whatever later moment it samples (fhsim/src/engine/model/OutputEvaluator.cpp:43-48).
Nothing in fhsim core initialises, zeroes or poisons that storage, and nothing asserts that the SimObject has. ISimObjectCreator::AddOutport takes only a name, a width and the function pointer (fhsim/include/fhsim/simobject/ISimObjectCreator.h:223), and the only rule its implementation enforces is where you may call it — "The SimObject tried to register output
ports outside of it's constructor" (fhsim/src/simobject/SimObjectOrganizer.cpp:399-403). The asymmetry with registered states is the sharpest way to put this: core fills the initial state vector with quiet_NaN (fhsim/src/engine/model/ModelAssemblyInitialConditions.cpp:117) and hard-errors if any slot is still NaN when assembly finishes (:441-442, BuildMissingInitialConditionError). A state without an initial value is a build failure; a port-backing member without one is silence. The class is on its own, and whether the member is written before the first read is decided by the engine's call ordering.
There are three ways an outport getter in this library is written, and only the first two are safe by construction:
CenterWeight::Position is return adX + m_IStatePos; (src/trawl/CenterWeight.cpp:88-91, and Velocity at :105-108); Actuator::OutLength and OutEnergy do the same (src/trawl/Actuator.cpp:81-84, :93-96). The initial value comes from the input file, so there is nothing to forget.ToTrawlDoor::OutRoll (src/trawl/ToTrawlDoor.cpp:22-38) and OutPitch (:41-57) derive the value from adX on every call; Crowfoot::LineTensionOut (src/trawl/Crowfoot.cpp:94-97) asks the underlying model. A variant of the same idea uses the core's ICommonComputation cache: SeineOperation::PWarpLength and SWarpLength call m_commonCalc2->ComputeFunction(dT, adX) before returning the member (src/seine/SeineOperation.cpp:175-185), and ComputeFunction runs the calculation once per port reset (fhsim/src/simobject/SerialCommonComputation.cpp:10-16, reset from fhsim/src/engine/model/ModelAssemblyService.cpp:19-27). This is the framework's own answer to the problem and it works.OdeFcn, AcceptedStep, or another port function of the same or a peer object. This is the unsafe pattern, and it is the whole of this issue.Pattern 3 is safe only if the constructor happens to write the member. Several classes do remember: TrawlDoorBase zeroes its six published position/velocity buffers and ma_dAlpha (src/trawl/TrawlDoorBase.cpp:36-45), Crowfoot zeroes m_instantPos, m_velocity and the line forces (src/trawl/Crowfoot.cpp:44-53), TrawlDoorAddedFoil zeroes m_foilEnergyConsumed (src/trawl/TrawlDoorAddedFoil.cpp:11). Four did not, and those four are FISH-0018, FISH-0021, FISH-0024 and FISH-0026. The discipline is per-author, undocumented and unenforced — which is the defect being filed.
The first observer sample is taken without any integration step. The state provider emits an initial snapshot and only evaluates the outputs (fhsim/src/engine/io/IntegratorStateProvider.cpp:142-166):
The regression harness runs through exactly this provider (fhsim/src/testtools/TestRunner.cpp:53), so the t=0 row of every _ref.csv in tests/in/ is that snapshot. Against that fixed point:
Written in OdeFcn — masked, but only by the variable-step path of FhIntegrator. FhIntegrator::Initialize calls InitialiseStepSize_ (fhsim/src/engine/integrator/FhIntegrator.cpp:52-60), which for a non-fixed step calls CalculateH0 (:152-162), whose first act is m_model->OdeFcn(T0, X0, m_xDot.data()); (:300), followed by ten more probe evaluations in the forward-Euler estimate loop (:312, kH0EstimateSteps = 10). That happens in the integrator constructor, before any output is ever evaluated. So in a variable-step run the member is written first and the garbage never shows — FISH-0026, Trawl/Actuator's Power, whose baseline tests/in/Actuator/Actuator_ref.csv:3 holds the correct 5.000000e+02. This rung needs correcting relative to the plain statement that "the first ODE evaluation
precedes the t=0 observer sample": it does not, in general. The masking is conditional on two things, and an input file controls both:
m_fixedStepSize = config.stepControl.step > 0 (fhsim/src/engine/integrator/FhIntegrator.cpp:120, with :156-160). A <StepControl> that sets Step takes the fixed-step branch, CalculateH0 is never called, and no OdeFcn evaluation precedes the t=0 sample. tests/in/Actuator/Actuator_in.xml:31 sets only StepMax, so the case is masked by luck of configuration.FhIntegratorSundials has no startup step-size estimate at all; its first OdeFcn-equivalent work happens inside Step (fhsim/src/engine/integrator/sundials/FhIntegratorSundials.cpp:159-208). Under a Sundials method, an OdeFcn-written outport is garbage at t=0 just like an AcceptedStep-written one.So rung 1 is masked in the default variable-step FhIntegrator configuration only, which is weaker than the ladder assumed and makes FISH-0026 a latent severity-3 rather than a tidiness item.
AcceptedStep — always garbage at t=0. m_model->AcceptedStep is called only from inside TryStep_, after a step has been computed and its error accepted (fhsim/src/engine/integrator/FhIntegrator.cpp:187-189), and likewise only inside Step on the Sundials path (.../FhIntegratorSundials.cpp:199). No accepted step can precede the initial snapshot, on any integrator, in any configuration. This rung is confirmed: it is FISH-0021, Trawl/PIDController's Out, recorded as the denormal 5.928788e-323.Written by another port call whose evaluation order is not fixed — garbage that varies with process memory layout. SeineOperation::BuoyPos returns m_buoyPos with its guard commented out (src/seine/SeineOperation.cpp:157-161, the dead // m_commonCalc->ComputeFunction(dT,adX); at :159), while its four sibling ports two functions below keep theirs (:165, :171, :177, :183). The member is written only by WarpConnected, so whether BuoyPos is valid depends on which port the observer or a peer reaches first. FISH-0024 records the resulting values: 0, 0, 0 when the case runs alone and 9.148902e-95, 3.479239e-278, 0 when it runs after six other cases in the same process. This rung is confirmed, and the commented-out guard makes it the clearest single instance in the library — the framework's remedy was present and was switched off.
FISH-0018's door path belongs on this rung with one qualification worth stating, because it changes what the item proves. ma_dAlpha is zeroed in the constructor (src/trawl/TrawlDoorBase.cpp:38), yet the observed t=0 AngleOfAttack is -8.090807e-03 in one run and 9.597028e-03 in another that differs only in the size of the process environment. Non-zero means it was written before the sample — the rung-1 masking worked — and different means it was written from an input that was itself indeterminate: the relative velocity is assembled from the door states and m_environment->GetParticleVelocity (src/trawl/TrawlDoorBase.cpp:265-278) and consumed at :345. So FISH-0018 is the demonstration that rung-1 masking is not protection: an early OdeFcn faithfully propagates an upstream port-backing member's garbage into a member that was initialised correctly. FISH-0018 is not localised (the upstream member may be in fhsim_environment or in core), and this item does not claim to localise it.
Two further read paths sit earlier than the t=0 sample and widen the exposure beyond the ladder:
InitialConditionSetup and FinalSetup on every SimObject with T hard-coded to 0 and the still-NaN initial-condition table as the state vector (fhsim/src/engine/model/ModelAssemblyInitialConditions.cpp:289, fhsim/src/engine/model/ModelAssemblyService.cpp:106). Input ports are live in that phase, so reading an inport re-enters an upstream outport getter before any integrator exists. Note that FISH-0024's recommended better fix — latch the vessel position in FinalSetup — lives in exactly this phase.SimulationManager at all (fhsim/src/engine/dll/common/DllCommon.cpp:107 calls FhSim::Step() directly, fhsim/src/engine/core/FhSim.cpp:119-124), so there is no automatic TStart snapshot and no CalculateH0 masking to rely on: a host that reads outputs before its first Step sees the raw member. Relevant if this library is ever consumed as a DLL or FMU.Part of why this family went unnoticed for four issues. A regression case compares each output column by a per-column RMS of the difference (fhsim/src/testtools/GeneralMethods.cpp:66-86):
Both comparison paths use it — NoRegression against a recorded _ref.csv (fhsim/src/testtools/TestResult.cpp:229) and CompareRegression (:277) — against a tolerance of 1e-10 (fhsim/include/fhsim/testtools/TestSpec.h:39, kDefaultRmsTolerance), which is what tests/TestUtils.h drives via RunAndCompareRegressionXmlTiming.
When the garbage is a denormal the RMS is not merely small, it is exactly zero: 5.928788e-323 * 5.928788e-323 underflows to 0.0, so squareSum is 0.0 and std::sqrt(0.0 / n) is 0.0. Verified numerically, and verified in practice while resolving FISH-0021: the full suite passed unchanged against the old baseline after the fix, and the defect was only ever visible by reading the recorded value. Nothing in the comparison path looks at whether a sample is finite or normal — std::isfinite appears in fhsim/src/testtools/ only in JacobianChecker.cpp:33,37,62, guarding its own arguments — and nothing in tests/ of this repository does either (grep for isfinite, isnan, denorm finds nothing).
The consequence is the part of this item that matters most for planning: the new test suite does not guard against this defect class at all. Thirteen passing cases and a full set of recorded baselines are compatible with every outport in the library publishing denormal garbage at t=0. A per-column RMS cannot fail on a denormal, and a baseline that is re-recorded from a run containing garbage pins the garbage.
No — adjacent, not a member, and the distinction is worth keeping. ValuesDisplay's m_valueDisplay is not behind an outport: nothing publishes it, it is a render-side pointer assigned only in RenderInit and deleted in the destructor. It shares the root discipline — a member that only an optional lifecycle callback writes, with no constructor initialiser — but not the mechanism (no return &m_member), not the ordering that decides it (the render lifecycle, not the integrator/observer schedule), and not the failure mode (undefined behaviour at teardown, not a wrong number on a port). It is cited here as evidence that the missing "initialise in the constructor" invariant is not confined to outports, and that is all. Do not fold it in; it is already resolved on its own terms.
A model author following the most natural reading of the outport API — "return a pointer to
where I keep the value" — writes a class whose published t=0 sample is indeterminate, and gets no warning from the compiler, the framework, the examples or the test suite. What the user then sees depends on where the member is written and on integrator configuration they may not have chosen:
t=0, consumed by whatever is wired to it. In examples/input/Example.xml that is the door roll-control loop, so the value enters a closed loop and the integrator propagates it (FISH-0021).The blast radius is every SimObject in the library, present and future, because there is no invariant to violate.
Three remedies, not alternatives to each other so much as three different depths. Only the first is in this repository's gift.
1. Document and apply an invariant in this library. In this repository; no core change. Write into AGENTS.md (and the library overview in doc/user/main_fishery.md) that a member whose address an outport getter returns is observable state at TStart, and that a class must therefore do one of the three safe things enumerated under The mechanism: return a state slice, compute into the member inside the getter (directly or behind ICommonComputation::ComputeFunction), or assign the member in the constructor. Then sweep the library once against it — the sweep is already done above, and the four instances are FISH-0021 (fixed), FISH-0024, FISH-0026 and, pending localisation, FISH-0018. Cheap, local, immediate, and it makes the next occurrence a review finding rather than a discovery. Its weakness is that it is unenforced: it catches nothing automatically and decays with author turnover. Note that SeineOperation shows the failure mode of a convention — the guard was written and then commented out (src/seine/SeineOperation.cpp:159).
2. Ask fhsim core to poison port-backing storage in debug builds. Spans another repository; cannot be done here. Core already has the pattern for states — NaN-fill plus a hard error for anything still unset at the end of assembly (fhsim/src/engine/model/ModelAssemblyInitialConditions.cpp:117, :441-442) — and this remedy is asking for the port equivalent. It knows every outport's function pointer and width (AddOutport), but not which storage backs it, so the honest version of this is narrower than "poison the buffer": the core would have to either require the class to register the storage (an API change) or check what a getter returns — e.g. a debug-build assertion in EvaluateOutputs and in the input-port read path that every published sample is finite and normal, reported with the object and port name. Combined with a signaling_NaN seed that the class opts into, that turns "depends on allocation luck" into a loud, named failure at the first sample. This is the only remedy that catches a class the author did not think about, and it is the most expensive: it touches core's output evaluation, needs a debug-only cost argument, and needs an owner in fhsim.
3. A comparison in the test harness that flags a non-finite or denormal sample instead of RMS-ing it away. Also fhsim core; cannot be done here. In fhsim/src/testtools/, have the column comparison report an issue when either the run or the reference contains a value that is not finite or is subnormal-and-non-zero, independently of the RMS. That is a few lines next to GetRootMeanSquareError (fhsim/src/testtools/GeneralMethods.cpp:66-86) or in TestResult's two comparison loops (.../TestResult.cpp:229, :277), it costs nothing at runtime, and it would have failed on tests/in/PIDController/PIDController_ref.csv:3 the day that baseline was recorded. It also protects against re-recording a baseline that contains garbage, which remedy 1 does not. It does not catch garbage that happens to be a plausible normal number — FISH-0018's 9.597028e-03 would pass it — so it is a detector for the denormal case specifically, which is the case the library has actually hit twice.
Owner question, stated as a scope decision: remedy 1 can be taken today, alone, by this repository, and it is recommended regardless of the rest. Remedies 2 and 3 are both changes to fhsim core and need a decision to open items against that codebase — remedy 3 is small, self-contained and high value for the effort; remedy 2 is the only real enforcement but is an API and evaluation-path discussion in core. Doing only remedy 1 leaves the library with a convention and no detector, which is the status quo plus a paragraph.
The mechanism is proved by reading, and the evidence above is the reading; the file:line citations reproduce at 1923368 in this repository and fa2dfc36 in fhsim. What is worth adding as a standing test, per remedy:
tests/in/Actuator/Actuator_in.xml, add Step="0.001" to its <StepControl> so the fixed-step branch is taken (fhsim/src/engine/integrator/FhIntegrator.cpp:120, :156-160), and compare the t=0 Power sample with the variable-step run. With m_power left uninitialised the two differ; after the one-line fix in FISH-0026 they agree at 0. This is the cheapest reproduction of the whole family and it needs no sanitiser.tests/in/PIDController/PIDController_ref.csv:3 containing 5.928788e-323. A denormal in either file must make the case fail; the current suite passes on it.-fsanitize=memory, or valgrind) over the suite reports every instance of the family directly, and is the tool FISH-0018 is waiting on. Worth running once against the whole of tests/in/ rather than per issue.Filing and documenting: none. Remedy 1 changes no code, so no baseline moves; applying the invariant to the remaining instances is the risk already assessed in FISH-0024 and FISH-0026 (both "very low", both making an indeterminate value deterministically zero, which is what a clean heap already produced, so no in-repo baseline moves).
Remedy 3 carries a real and worth-stating risk: turning it on may fail cases that pass today, because a recorded baseline containing a denormal becomes a failure the moment the check exists. That is the point of it, but it means the change must be sequenced after the known instances are fixed and the baselines re-recorded, or it will land as a wall of red.
Remedy 2 is the largest risk of the three, in the other direction: a debug-build assertion in core's output evaluation is a change to the shared engine that every downstream library feels, and a signaling_NaN seed changes what a release build of a broken class does (from silent garbage to a NaN that propagates). Both need the fhsim owner, not this one.
Leaving all three undone is not neutral: the library keeps a defect class that the test suite cannot see, and the next instance will again be found by noticing an odd number in a CSV.
1.9.8