Skip to content

halcompile: allocate fake params for personality-masked params - #4637

Open
grandixximo wants to merge 2 commits into
LinuxCNC:masterfrom
grandixximo:bldc-4625-comp-guard
Open

grandixximo wants to merge 2 commits into
LinuxCNC:masterfrom
grandixximo:bldc-4625-comp-guard

Conversation

@grandixximo

@grandixximo grandixximo commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Reshaped per @BsAtHome's review on #4626: the fix belongs in halcompile, using the fake allocator, not an instance-side backing store.

  1. Reverts the backing-store change merged in f43ab82.
  2. halcompile now emits hal_param_new_fake() in the else branch of the personality guard, so a masked param's handle always points at HAL-owned zeroed storage; exported or not, reads return zero instead of dereferencing NULL.

bldc.comp needs no change. Verified: cfg="a" completes case 0x02 (phase-angle tracks the 90 degree lead), cfg="hq" unchanged, full tree rebuild clean.

Fixes #4625.

@BsAtHome

BsAtHome commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

The correct fix in bldc would be to fix the calls to hal_param_new_X().

When a personality mask/confition is specified on the component's param declaration, then the allocation should be the target type or a fake.

The generated code in bldc:

if(personality & 0x04) {
    r = hal_param_new_sint(comp_id, HAL_RO, &(inst->offset_measured_p), 0,
        "%s.offset-measured", prefix);
    if(r != 0) return r;
}

Should in reality have been:

if(personality & 0x04) {
    r = hal_param_new_sint(comp_id, HAL_RO, &(inst->offset_measured_p), 0,
        "%s.offset-measured", prefix);
} else {
    r = hal_param_new_fake(comp_id, (hal_refs_u *)&(inst->offset_measured_p));
}
if(r != 0) return r;

That would have fixed it once and for all, including for the users. This can be done in halcompile and no change in bldc.comp required.

A param masked out by personality was never allocated, so its accessor
handle stayed NULL and reading it killed the realtime thread (LinuxCNC#4625).
Allocate a fake param in the personality else branch, per BsAtHome's
review: the handle always points at HAL-owned storage, exported or
not, and a masked param reads back zero.
@grandixximo
grandixximo force-pushed the bldc-4625-comp-guard branch from 7f07f66 to 3624a45 Compare October 5, 2026 13:08
@grandixximo grandixximo changed the title Revert halcompile masked-param storage; fix bldc at the source halcompile: allocate fake params for personality-masked params Oct 5, 2026
@grandixximo

Copy link
Copy Markdown
Contributor Author

@BsAtHome Shaped as you said: revert plus hal_param_new_fake() in the personality else branch. The generated code now matches your sketch exactly, bldc.comp untouched.

One open point from the earlier review round. @rene-dev argued on #4626: "imho it still is a logic error if you read a pin that you didnt create, and returning 0 masks this error." With the fake allocation the read still returns 0 silently. Do you want a warn-once message on first access of a personality-masked param, or is silent zero acceptable?

@BsAtHome

BsAtHome commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

One open point from the earlier review round. @rene-dev argued on #4626: "imho it still is a logic error if you read a pin that you didnt create, and returning 0 masks this error." With the fake allocation the read still returns 0 silently. Do you want a warn-once message on first access of a personality-masked param, or is silent zero acceptable?

And that is exactly the gremlin we have been fighting while doing the getter/setter conversion.

Parameters have been used as (very) expensive variables and that should be fixed. The fake parameter was introduced as a stopgap to keep current code running. However, it still needs review and fixing. In the end, that fake allocation needs to go. We need to be firm and tell everybody who uses params (and pins) as storage that it will not work. For C-based code, this is done for some instances already. For .comp based code, well, there are a few more things we need to look at.

@grandixximo

Copy link
Copy Markdown
Contributor Author

One more shape for the longer term, meeting input if nothing else. The out-of-tree problem does not go away with roadmap firmness: a .comp author we cannot reach, reading a personality-masked param, gets a dead realtime thread once fakes are removed, with no diagnostic.

NULL-checked accessors cover it without any storage. For personality-guarded params, halcompile could generate:

(h ? hal_get_sint(h) : (warn_once("param 'x' accessed but masked by personality"), 0))

No backing store, no fake, one truth. The logic error is loud (@rene-dev's point from #4626), fakes can die on schedule, and out-of-tree comps degrade to a logged warning and zero instead of a SIGSEGV. Cost is one predictable branch per access plus a byte of once-state per guarded param.

Not asking to hold this PR for it; #4637 as-is is the right fix today. Candidate for the "when fakes go" milestone.

@BsAtHome

BsAtHome commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

This adds a condition to each and every access. That is problematic.

We must first ensure that we do not have any cases in-tree before we can put something like this to warn out-of-tree. And then, we need to have a clear path how this pattern must be rewritten such that the condition+warning goes away. That is the really tricky part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Segmentation Violation introduced by hal: Update last set of .comp components to getter/setter.

2 participants