halcompile: back masked params with zeroed storage, warn on access - #4626
Conversation
e559bf7 to
6043ace
Compare
grandixximo
changed the title
|
I didnt even know that you could do that. imho it still is a logic error if you read a pin that you didnt create, and returning 0 masks this error. |
A param masked out by personality is never exported, so its accessor handle stayed NULL; reading it dereferenced NULL and killed the realtime thread (LinuxCNC#4625). Pre-conversion such params were plain struct fields and read back zero. Point each param's handle at a zeroed instance-local backing store before the guarded hal_param_new_*() call, restoring the old behavior. Reading 0 can hide a genuine logic error, so a personality-masked param also gets a masked flag, set when the export is skipped; the generated accessors print a one-time message naming the param on first access. Fixes LinuxCNC#4625.
d2c63bd to
01bcbf7
Compare
|
Fair point, silently returning 0 hides the bug in the component. Reworked the commit: the zeroed backing store still restores the pre-conversion read-as-zero behavior (that part is the crash fix), but a personality-masked param now sets a flag when its export is skipped, and the generated accessors print a one-time message on first access, naming the param: It uses RTAPI_MSG_ERR because RTAPI_MSG_WARN is below the default message level and would never be seen. Once only, so no log flood in the thread. So the crash is gone, the old behavior is back, and the logic error is now loud instead of masked. Verified with bldc cfg="a" (message prints once, case 0x02 completes) and cfg="hq" (no message). |
01bcbf7 to
3822d1e
Compare
grandixximo
changed the title
For a pin I agree with you more than for a param. But you could take the position that personality just chooses whether an internal variable is exposed to HAL or not. |
3 participants
Reworked after @andypugh's comment on #4625: the fix belongs in halcompile, not in the component.
With getter/setter accessors, a param masked out by personality keeps a NULL handle (only hal_param_new_*() fills it), so a component reading it derefs NULL and kills the realtime thread. Before the conversion, params were plain instance-struct fields and such reads returned zero. That is how #4625 happened: bldc case 0x02 reads offset_measured, which only exists when personality has bit 0x04, so cfg="a" (and "ha"/"ai"/"hai" via the redirect cases) crashed on the second pass.
halcompile now emits a zeroed instance-local backing store per param and points the handle at it before the personality-guarded hal_param_new_*() call, restoring the pre-conversion read-as-zero behavior for any masked param in any generated component.
After @rene-dev's review: silently returning 0 can hide a genuine logic error, so a personality-masked param also sets a flag when its export is skipped, and the generated accessors print a one-time message on first access, naming the param. It uses RTAPI_MSG_ERR because RTAPI_MSG_WARN is below the default message level and would never be seen.
Verified: bldc cfg="a", "ha", "ai" complete case 0x02 with the message printed once; cfg="hq" and "q6" behave as before with no message. Full tree rebuild regenerates and compiles all components cleanly.
The sserial hardening commits that were briefly on this branch are now their own PRs.
Fixes #4625.