halcompile: reject declarations that export the same HAL name by tzuohann · Pull Request #4298 · LinuxCNC/linuxcnc

tzuohann · 2026-07-30T18:35:43Z

pin in float my_input is exported as component.N.my-input — underscores become dashes (comp.adoc, HALNAME).

A component that declares both x_y and x_y_ exports both as x-y. halcompile accepts it; the module then fails at loadrt:

HAL: ERROR: duplicate variable 'collide.0.x-y'
collide: rtapi_app_main: Invalid argument (-22)

check_name_ok() only compares declared names. check_hal_name() rejects the collision at the offending line and points at the HALNAME documentation. Functions are tracked separately from pins and params, which share one namespace in hal_lib.c.

Docs: NAMES section in the halcompile man page, note under the HALNAME table in comp.adoc. Tests: tests/halcompile/halname. All 133 in-tree .comp files preprocess with no new output.

Backport for 2.9: #4299.

🤖 Generated with Claude Code

BsAtHome · 2026-07-30T19:14:27Z

Name collisions are bad by default and fail to compile. Why is there an option for this? You should be able to check this much easier using a parallel name array for target names (which you apparently do) without complex regexes or lambdas by using a simple if name in array construct. Also note that functions have a different HAL namespace than pins/params.

Your second PR seems to be a duplicate and changes a generated (man) file that is not part of the repository. Why is this submitted twice?

grandixximo · 2026-07-31T00:22:27Z

Your second PR seems to be a duplicate and changes a generated (man) file that is not part of the repository. Why is this submitted twice?

It's a 2.9 backport, no .adoc there, also threw me off...

tzuohann · 2026-07-31T01:01:23Z

thanks. help me understand here. is this is a problem that can throw some people (amateurs using AI) off? if so and a little fix can help, I'll try to sharpen the solution. but if its not even an issue, i'll close the PR.

grandixximo · 2026-07-31T01:15:06Z

Name collisions are bad by default and fail to compile.

But they actually don't, I tested the claimed x_y + x_y_ on master, compiles fine, fails on loadrt...

Edit:
Unless you mean name collisions should always fail to compile, in the PR context, then agreed...

BsAtHome · 2026-07-31T14:22:48Z

Still, the option is useless.
When you encounter the situation, then you have an error. Either immediately at compile or afterwards at loadrt. There is no point in hiding the message. Halcompile should simply return with an error so that the build is interrupted.

tzuohann · 2026-07-31T17:06:52Z

Still, the option is useless. When you encounter the situation, then you have an error. Either immediately at compile or afterwards at loadrt. There is no point in hiding the message. Halcompile should simply return with an error so that the build is interrupted.

ok I think this resolved the bit of confusion I as well as @grandixximo had. so this is a little problem that should be patched. but the option of hiding it is useless. i'll resubmit removing the option to hide it. thanks @BsAtHome

grandixximo · 2026-08-01T03:31:11Z

Wasn't this initially showing also info that the pins you will find in hal have different names?
I think a general one liner info after compilation would suffice. if it's one line per compilation, we can also keep it in the normal build, no flag needed? probably a separate PR/discussion from the collision.

tzuohann · 2026-08-05T04:25:12Z

Wasn't this initially showing also info that the pins you will find in hal have different names? I think a general one liner info after compilation would suffice. if it's one line per compilation, we can also keep it in the normal build, no flag needed? probably a separate PR/discussion from the collision.

Per component: that's already what it did — one line per file with the names collapsed into it (my_input -> my-input, ...). But the build runs halcompile once per .comp, so its still 100+ lines on a full master build.

One line for the whole build: It would have to be a fixed message from src/hal/components/Submakefile, which then can't name any actual pins. Also, in-tree only, so out-of-tree authors never see it.

I have no clue how folks use this so I'll leave it to you guys to tell me what to do.

grandixximo · 2026-08-05T05:38:03Z

that's already what it did — one line per file with the names collapsed into it (my_input -> my-input, ...

You did do that originally, but that part has been removed, that is what I was saying, it is not there anymore, check the code...
halcompile is currently silent on success which is typical for compile tools, although it could be argued the audience of halcompile is not typical programmer audience, anyone using a compile tool should at least read the --help, a short section in the help about how names get managed, could avoid the confusion, and keep the build clean.
My proposal for help addition is:

Names:
    Declared names are C identifiers; HAL exports them with underscores
    replaced by dashes.  'pin in float my_input' is reached as
    'component.N.my-input'.  After loadrt, 'halcmd show pin component'
    lists the exported names.

BsAtHome · 2026-08-05T08:46:16Z

Isn't this already described in https://linuxcnc.org/docs/devel/html/en/hal/comp.html ?

grandixximo · 2026-08-05T09:07:46Z

Correct, but the OP, before edits (the whole reason this PR started), was because the user and his AI missed that, the original proposal was a noisy my_pin =>my-pin to stdout silenced with a flag, basically an in your face "WARNING: names have changed", displayed even for those who know this, and those who reads the docs, arguably useful for newcomers, but an eyesore for veterans.
I think within the original motivation there was an acceptable argument: this behavior is only documented in the comp.adoc

An addition in the --help will improve visibility, and will weaken the "is not well documented" argument, after the addition in --help there are less excuses for missing it, AI or human...

BsAtHome · 2026-08-05T09:20:55Z

The --help is there to remind you of command line switches. Not to tell you how to write content. That belongs to the man pages and other documentation. The man-page is terse and should link properly to the docs.

And, as you correctly said, no new is good news and errors/warnings are supposed to be one-liner messages with the correct information (in the correct format for IDEs to use).

_{Relying on abnormal intelligence to do the right thing is like asking an amoeba to write Shakespeare. The letters are there but the meaning is lost. As they say, gigo.}

grandixximo · 2026-08-05T11:06:45Z

The man page argument has a hole for exactly the audience halcompile serves: component developers. They overwhelmingly work from a RIP build (git clone, make, rip-environment), and RIP installs nothing to MANPATH. For them man halcompile returns "No manual entry" unless they also built the docs tree locally. The deb path (linuxcnc-uspace-dev) ships the man page, but that package is for people compiling components against an installed system, not the people writing them.

--help is the only documentation guaranteed to be present in every install flavor, at the exact moment someone unfamiliar with the tool looks for orientation.

There is also in-tree precedent for non-switch guidance in this very usage() text: "Do not use [sudo] for RIP installation" is behavioral advice, not a switch reminder.

Not proposing a tutorial, just 4 lines stating the one non-obvious semantic (C identifier => dashed HAL name) plus where to see the result (halcmd show pin). The full HALNAME rules stay in comp.adoc and the man page; the help text would end by pointing there. Zero build noise, zero runtime cost, and it covers the RIP user the man page never reaches.

BsAtHome · 2026-08-16T20:59:57Z

+# A name that is exported under a different HAL name must still compile.
+rm -f halname_mangled.c
+halcompile --preprocess halname_mangled.comp 2>&1
+test -f halname_mangled.c || echo 'halcompile failed to produce halname_mangled.c'


Why is this tested? Isn't the normal halcompile functionality already tested elsewhere?

Removed. tests/halcompile/names covers it.

BsAtHome · 2026-08-16T21:00:34Z

+pin in bit x_y;
+pin out bit x_y_;


You have a check for pins, params and functions. However, you only test pins here. Why not the other cases?

I went a little further:

TLDR;
Added all the cases. Doing that showed the check was wrong in both directions, so it now compares the set of HAL names a declaration claims rather than its mangled name.

A little more detail:

Missed — halcompile accepted these, then loadrt failed:

pin in bit x_#[4]; claims x-0 .. x-3 pin out bit x_0; claims x-0 function f; hal_export_funct creates pin f.time pin out s32 f.time; claims f.time

Wrongly refused — halcompile rejected these, but they never coexist:

pin in bit x_y if personality == 1; pin out bit x_y_ if personality == 0;

Tests now cover pin/pin, pin/param, function/function, array/scalar, function/derived-pin and same-condition, plus the two that must be accepted.

BsAtHome · 2026-08-16T21:05:12Z

+        Error("'%s' and '%s' both export the HAL name '%s'; see HALNAME under "
+              "'Syntax' in the Halcompile HAL Component Generator "
+              "documentation, https://linuxcnc.org/docs/html/hal/comp.html"
+              % (seen[hal_name], name, hal_name))


The verbosity needs to be reduced. Maybe:
"Name collision: %s '%s' and '%s' both become '%s' and are indistinguishable."
The first %s argument should be one of "pin", "param" or "function" to indicate type.

Done, your wording. One tweak: The type is attached to each name rather than leading:

Name collision: pin 'x_y' and param 'x_y_' both become 'x-y' and are indistinguishable.

URL dropped — it is in the man page now.

BsAtHome · 2026-08-16T21:06:57Z

+See HALNAME under _Syntax_ in the _Halcompile HAL Component Generator_
+documentation for the full rules.


Can this link to the documentation?

Linked. comp.adoc gains a [[sec:halname]] anchor at the HALNAME table, and NAMES points at it:

Full rules: HALNAME under Syntax in the HAL Component Generator
documentation, https://linuxcnc.org/docs/html/hal/comp.html#sec:halname

An explicit anchor rather than the auto-generated #_syntax, which breaks if the heading is ever renamed.

BsAtHome · 2026-08-26T08:05:35Z

What is the status of progress? There are still some unaddressed open comments.

grandixximo · 2026-08-26T09:27:42Z

@tzuohann is on vacation till mid September, just texted him on Discord...

tzuohann · 2026-09-18T15:17:43Z

hi guys, back. i'll double check these before end of weekend.

A name declared in a .comp file is a C identifier, but it is exported under a mangled HAL identifier: underscores become dashes and a trailing dash or period is removed (comp.adoc, HALNAME). check_name_ok() compares only declared names, so a component declaring both x_y and x_y_ exported both as x-y. halcompile accepted it and the module failed at loadrt: HAL: ERROR: duplicate pin 'collide.0.x-y' collide: rtapi_app_main: Invalid argument (-22) A declaration does not claim one name, so comparing mangled declared names is not enough in either direction: - an array claims one name per element, and to_hal() turns the "#" block into a printf conversion, so 'x_#[4]' and 'x_0' both claim x-0 while their mangled forms never compare equal. The [MAXSIZE : CONDSIZE] form varies only the element count, not whether the loop runs, so every element up to MAXSIZE is claimed; - hal_export_funct() creates <funct>.time, .tmax and .tmax-increased in the pin and param namespace, so 'function f' and 'pin f.time' collide although only one of them is written down; - a declaration carrying an 'if' condition exists only for some values of personality. Two under DIFFERENT conditions can be mutually exclusive -- 'if personality == 0' against 'if personality == 1' loads today -- and the expressions cannot be evaluated here, so that pair is left alone. The same condition on both sides is still a collision, and an empty or constant 'if' is not a condition at all, matching the guard the code generator emits. claim() therefore records the set of HAL names each declaration takes. Pins and params share one set, because hal_lib.c refuses a pin whose name is already a param and the reverse; functions have their own. Docs: NAMES section in the halcompile man page, linking to the HALNAME rules in comp.adoc, which gains a [[sec:halname]] anchor to link at. Tests: tests/halcompile/halname covers pin/pin, pin/param, function/function, array/scalar, function/derived-pin and same-condition collisions, and asserts that a pin and a function mangling to one name are accepted, since their namespaces differ, as are two declarations under different personality conditions. All 146 in-tree .comp files preprocess to the same C, and with identical exit status, against master.

tzuohann · 2026-09-18T20:32:50Z

What is the status of progress? There are still some unaddressed open comments.

All four addressed, and the branch is rebased onto master — it was 645 commits behind.

Widening the tests as you asked turned up that the check itself was wrong in both directions, so it is rewritten: it now compares the set of HAL names a declaration claims, not its
mangled name. Details in the thread on the test file.

Force-pushed. All 146 in-tree .comp files preprocess to the same C, with the same exit status, as master.

BsAtHome · 2026-09-18T21:31:35Z

+pin in bit x_#[4];
+pin out bit x_0;


All pin/param types have been changed and the old ones will soon be removed completely. You need to update to the new names or deprecation warnings will be issued:

bit -> bool

float -> real

s64 -> sint

u64 -> uint

s32, u32, signed, unsigned -> soon to be removed and mapped to sint/uint

BsAtHome · 2026-09-18T22:03:00Z

+    Two declarations guarded by DIFFERENT 'if' conditions are left alone: they
+    need not ever exist together -- 'if personality == 0' against
+    'if personality == 1' is a component that has always loaded -- and the
+    expressions cannot be evaluated here.  The same condition on both sides is
+    a collision, since whatever makes one exist makes the other exist too.


You never know whether they are different unless you actually parse the C code and evaluate the AST. The test is a false sense of security since you are not doing that.

Writing personality derived pins that overlap is bad design. So I'm not sure we should bother and simply fail when you detect a collision.

BsAtHome · 2026-09-18T22:12:05Z

+# A pin array claims one HAL name per element.  The bound keeps a nonsense
+# size from expanding into a MemoryError instead of a diagnostic; an array
+# larger than this cannot be loaded anyway, since every element is a separate
+# HAL object in shared memory.
+ARRAY_CLAIM_LIMIT = 4096


Where does this limit come from? The default personality limit is 64 and max 256 IIRC (a byte size). Therefore, personality limited arrays never are larger than 64.

For normal arrays, if someone creates an array of 4097 pins, should that fail? The current HAL memory allocation can support many more pins than 4096.

From a practical standpoint, having really many pins in an array would probably indicate some kind of error or abuse. I'd probably say it is an error and we may want to limit it to the same as the personality limit of 256. Unless there is a really good reason not to... You have one?

BsAtHome · 2026-09-18T22:12:49Z

+    For the '[MAXSIZE : CONDSIZE]' form the element count varies with
+    personality but the loop that creates them is not guarded, so every
+    element up to MAXSIZE is claimed: any of them can exist.


The loop isn't guarded in the component either...

BsAtHome · 2026-09-18T22:13:43Z

+    """Every HAL name one declaration can claim.
+


The function "doc" string is not a doc string but a comment of what you are trying to achieve. Thus, it is a comment.

BsAtHome · 2026-09-18T22:15:50Z

+def condition_of(personality):
+    """The 'if' expression guarding a declaration, or None when it is always
+    created.
+


You need to evaluate the code to know...

BsAtHome · 2026-09-18T22:28:52Z

+# is still a collision: whatever creates one creates the other.
+for c in collide_pin_pin collide_pin_param collide_function \
+         collide_array collide_funct_time collide_same_condition; do
+    rm -f $c.c


You should quote this rm -f "$c.c".
Also other places.

BsAtHome · 2026-09-18T22:32:51Z

+    if [ ! -f $c.c ]; then
+        echo "halcompile failed to produce $c.c"
+    fi


Nitpicking...
You are using bash. Alternative formulation:
[ -f "$c.c" ] || echo "Nothing was produced"

Or inverse:
[ -f "$c.c" ] && echo "Something got produced"

BsAtHome · 2026-09-18T22:59:03Z

And, your work is much appreciated. Detecting duplicates is a hard problem and solving it is quite involved. That is why I'm looking deep.

tzuohann force-pushed the halcompile-halname-upstream branch from ea6413a to e1db816 Compare July 30, 2026 18:45

tzuohann mentioned this pull request Jul 30, 2026

halcompile: reject declarations that export the same HAL name (2.9 backport) #4299

Open

tzuohann force-pushed the halcompile-halname-upstream branch from e1db816 to 1188057 Compare July 31, 2026 01:12

tzuohann force-pushed the halcompile-halname-upstream branch from 1188057 to 6b100fc Compare July 31, 2026 17:13

tzuohann changed the title ~~halcompile: warn about, and reject colliding, mangled HAL names~~ halcompile: reject declarations that export the same HAL name Jul 31, 2026

BsAtHome reviewed Aug 16, 2026

View reviewed changes

tzuohann force-pushed the halcompile-halname-upstream branch from 6b100fc to edf5da2 Compare September 18, 2026 20:32

BsAtHome reviewed Sep 18, 2026

View reviewed changes

		See HALNAME under _Syntax_ in the _Halcompile HAL Component Generator_
		documentation for the full rules.

Conversation

tzuohann commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BsAtHome commented Jul 30, 2026

Uh oh!

grandixximo commented Jul 31, 2026

Uh oh!

tzuohann commented Jul 31, 2026

Uh oh!

grandixximo commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BsAtHome commented Jul 31, 2026

Uh oh!

tzuohann commented Jul 31, 2026

Uh oh!

grandixximo commented Aug 1, 2026

Uh oh!

tzuohann commented Aug 5, 2026

Uh oh!

grandixximo commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BsAtHome commented Aug 5, 2026

Uh oh!

grandixximo commented Aug 5, 2026

Uh oh!

BsAtHome commented Aug 5, 2026

Uh oh!

grandixximo commented Aug 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzuohann Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BsAtHome commented Aug 26, 2026

Uh oh!

grandixximo commented Aug 26, 2026

Uh oh!

tzuohann commented Sep 18, 2026

Uh oh!

tzuohann commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BsAtHome commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

tzuohann commented Jul 30, 2026 •

edited

Loading

grandixximo commented Jul 31, 2026 •

edited

Loading

grandixximo commented Aug 5, 2026 •

edited

Loading

tzuohann Sep 18, 2026 •

edited

Loading

tzuohann commented Sep 18, 2026 •

edited

Loading