Repository navigation
libnml: use int64_t instead of long for CMS/NML sizes and ids - #4614
Conversation
b9ab915 to
04cafcc
Compare
|
Seems I have a deja vu, I thought that particular thing with ambiguous overloads was already fixed? |
|
You are thinking of #3803, which you merged yourself. That one replaced the |
|
Problems for 32bit architectures will keep to creep in, if we want to keep that we will need a 32bit CI runner (again) sooner or later. |
|
I really don't like the casts to long long. More importantly, who in their right mind needs to build/use this on a i386/i686 machine? Then, using an armhf instead of an arm64 is equally mind boggling. All machines that are powerful enough to run a modern version of LCNC are all 64-bit machines. We've had amd64 for more than two decades and arm64 for well over a decade. Any modern hardware you buy is 64-bit capable. Why should we be willingly torture ourselves here to support this old stuff? |
|
Discussion for the meeting, point number one on the agenda next week, no urgency, could stay here till someone actually complains... |
Me neither, but I deleted my comment. Those are arguments to printf-type arg list with format specifier "%lld", so that really should be a (long long int). (Or, better yet, the printf could be replaced with fmt). |
Now, that is a thought! :-) |
|
To be clear on intent: this PR is not a request to keep supporting 32-bit, and I am not pushing for merge. The question for the meeting is only whether we want the tree to still build on ILP32 or not. If yes, this is the entire cost today: 18 lines in libnml, everything else on master already builds. If the meeting decides 32-bit is dead, closing this costs nothing either. What it is not: no CI runner, no support commitment, no promise to fix future 32-bit breakage. On the |
|
@grandixximo IMHO it is OK as it is and can go in, anything more than absolutely necessary on this dead and hopefully soon to be removed code (NML) is a waste of resources. |
04cafcc to
e0ede61
Compare
|
I also dont see the point, libnml will be removed anyway, but it can go in. |
Except for the casting in halcmd. If you are concerned, move it all to libfmt and lets be done with it (in a separate PR).
Due to legacy design decisions... They usually cause entire neighbourhoods to be haunted and abandoned. Changing it requires every in-tree and out of-tree build and UI to be changed. The in-tree version can be made clean with some Spengler ingenuity and Stantz magic, if there is someone to test on real hardware (don't ask Venkman). The out-of-tree stuff, well, that is a different question. |
e0ede61 to
5290dee
Compare
Since efe4ef2 the CMS_UPDATER overloads are fixed-width. On LP64 long == int64_t so callers passing long still compiled; on ILP32 long matches no overload and the i386 build fails (260 errors, all in libnml). Use explicit int64_t at the affected declarations. No functional change on 64-bit. Side effect: the NML encoded header layout no longer depends on the width of long.
The plasmac low_cut_volts print used %ld with a hal_get_sint() value (int64_t), which only matches on LP64. Use PRId64, since rtapi_print_msg is the only output channel in a realtime component. The kernel branch of rtapi_stdint.h has no inttypes.h, so give it PRId64/PRIu64 fallbacks matching printk's long long s64.
5290dee to
6b72db8
Compare
|
At the meeting it was decided to keep support for 32bit cause of beaglebone, probably keeping support till 2030 as armhf is going to be supported by debian and kernel till then, since we are close to a release this should go in, different reservations can be discussed next meeting, or here. |
So who is adding it to CI? |
|
I'll see what I can do |
|
This actually exposes an issue no one has talked about: Linuxcnc works on the assumption that hal pin access is atomic. 64 bit integer access is not atomic on 32 bit, so realistically we have to drop all 32 bit support now. |
32-bit support stays (BeagleBone and similar armhf boards), but nothing in CI builds for an ILP32 target, so breakage like the libnml long vs int64_t overload errors fixed in LinuxCNC#4614 goes unnoticed until someone builds on such a machine. Add a package-arch-32bit job that runs the same steps as package-arch (build the arch Debian packages, install them, run the test suite) in a Debian trixie container for i386 and armhf. The GitHub "container:" key cannot be used for these images because the runner injects its own 64-bit node into the container to run actions, so the job checks out on the host and starts the container with "docker run --platform". i386 runs natively on the x86_64 runner. armhf runs on the arm64 runner, natively if the CPU can execute AArch32, otherwise through QEMU.
This issue have been exposed for as long as the floating point interface has been There is something that could be done about it, at least partially. Not that it should be done. For all HAL accesses you now have a getter/setter interface, where you can rewrite them to use atomic access. That still does not solve atomicity because add/sub to the values are still not implemented in an atomic way. Fixing that would mean a lot of code changes. Some would be very complex because reads and writes can be spread out very far (split brain problem). Therefore, the atomic access you describe is currently a fantasy on both 32- and 64-bit systems, regardless whether or not we read/write the pins atomically. Fixing any of this also entails the assumption that 64-bit atomic operations exist on 32-bit architectures. That may not be a given either. If you don't have proper 64-bit primitives on 32-bit architectures, then you can't even fix the operations on double. |
|
@rene-dev good catch, it is real, but the fix is small enough that I don't think it means dropping 32-bit. On i386 the 64-bit integer accessors compile to two 32-bit moves, and a test with the master accessors does see torn ±2^32 values. Relaxed atomics in the getters/setters fix it (zero torn reads in the same test), compile to the same instructions as today on amd64/arm64, and the primitives exist on every Debian 32-bit target (i586+, ARMv7 @BsAtHome on CI: I have i386 and armhf jobs (Debian trixie, armhf runs natively on the arm64 runner) green in my fork. They already caught two 32-bit bugs, so they earn their keep. I'll open the PR, plus a small one for the atomic accessors if you're OK with me touching them. If you still want to drop 32-bit, that's fair to bring to the next meeting; this way we would at least know what it costs. |
In order to allow atomics to be effective, we need to test for the availability of proper 64-bit primitives in configure when targeting a 32-bit arch. We cannot rely on everybody using stock Debian and derivatives and homemade may have other ideas. |
|
Am Montag, dem 12.10.2026 um 09:08 -0700 schrieb BsAtHome:
BsAtHome
left a comment (LinuxCNC/linuxcnc#4614) [1]
> This actually exposes an issue no one has talked about: Linuxcnc
> works on the assumption that hal pin access is atomic. 64 bit
> integer access is not atomic on 32 bit, so realistically we have to
> drop all 32 bit support now.
This issue have been exposed for as long as the floating point
interface has been double. That has never been atomic on 32-bit and
now the rest isn't either.
There is something that could be done about it, at least partially.
Not that it should be done. For all HAL accesses you now have a
getter/setter interface, where you can rewrite them to use atomic
access. That still does not solve atomicity because add/sub to the
values are still not implemented in an atomic way. Fixing that would
mean a lot of code changes. Some would be very complex because reads
and writes can be spread out very far (split brain problem).
Therefore, the atomic access you describe is currently a fantasy on
both 32- and 64-bit systems, regardless whether or not we read/write
the pins atomically.
Fixing any of this also entails the assumption that 64-bit atomic
operations exist on 32-bit architectures. That may not be a given
either. If you don't have proper 64-bit primitives on 32-bit
architectures, then you can't even fix the operations on double.
It is not that big of a problem as it may seem at first glance. First,
there is not much real concurrency on a typical system. With one
realtime thread (which seems to be typical on beagle bone because it
uses the PRU as step generator) there is no concurrent access to HAL
pins from the realtime stuff. So motion related stuff should be OK.
Problematic is that non-realtime stuff may read wrong values.
read/write of doubles on ia32 is atomic if doubles are 8-byte aligned
which the compiler does unless forced not to. not on armhf and also
64bit ints (which didn't exist until 2.10) are not, that is a problem
in case of negative numbers or numbers changing the upper 32bits. one
more argument to get rid of integers alltogether 😉
non-atomic add/subtract should not be more of an issue than non-atomic
read/store. reading/writing atomically should be enough, the compiler
can't optimize that away and do some "direct in-place add", and AFAIK,
that's not even possible in the ARM world, there you always have to
load into a register, add, and store again.
with a base and servo thread and a typical stepgen config on 32bit arm,
you would in theory have concurrent access to the inputs/outputs of
stepgen where you could read values where one half has been updated
while the other has not. Would be interesting to investigate how often
that occurs in practice (i guess not very often) and what the
consequences are. usually, target and feedback values are not changing
that much in one servo cycle, so most of these updates will probably
not even be far off.
A warning could/should be issued if you start more than one realtime
thread on arm 32bit.
In theory there
ist https://en.cppreference.com/c/header/stdatomic stdatomic.h
(available since C11), the types declared there are atomic for basic
operations like assignments or additions but will internally use a lock
on armhf32 which incurs a function call and mutex get/set and possibly
spinning lock on each access.
In our case of limited concurrency something like "read value, read
again and loop if it changed" could suffice, that could have acceptable
performance impact (that scheme falls apart if there could be more than
one update while looping which AFAICT is not possible in our case).
Maybe it really is time to retire 32bit support for 2.10 and keep a
linuxcnc2.9 package around.
[1] (LinuxCNC/linuxcnc#4614)
#4614 (comment)
|
That would be a purely academic issue. All accesses to HAL memory are volatile and no implicit read-modify-store instruction is allowed since the introduction of getter/setter. Adding/subtracting is non-atomic. But, newer ARM arch has a form of atomic add/sub (or you can't implement If there would be introduced add/subtract setters, then these operations would also be properly atomic. However, that is an issue that is very much larger because the code base is written without any attention to atomicity and distributes read-modify-write cycles. It isn't that big an issue as long as read/write is atomic.
Yes, and that is why I insisted on a configure check that ensures native 64-bit load/store atomics on all architectures. That would drop the older ARM arch at that step, which is the right call IMO. |
|
Am Donnerstag, dem 15.10.2026 um 03:03 -0700 schrieb BsAtHome:
That would be a purely academic issue. All accesses to HAL memory are
volatile and no implicit read-modify-store instruction is allowed
since the introduction of getter/setter. Adding/subtracting is non-
atomic. But, newer ARM arch has a form of atomic add/sub (or you
can't implement __atomic_add_fetch and friends). Whether these are
64-bit is testable.
If there would be introduced add/subtract setters, then these
operations would also be properly atomic. However, that is an issue
that is very much larger because the code base is written without any
attention to atomicity and distributes read-modify-write cycles. It
isn't that big an issue as long as read/write is atomic.
Can you give an example where these atomic add/subtract setters would
be needed? I don't see how you could see some non-completed addition if
you have atomic read and atomic write.
The code fetches the volatile value (atomically) into a register, adds
something, and stores the value (atomically) back into memory. So at no
point does some non-atomicity of the addition appear anywhere on a HAL
pin. That could only occur if you use global variables to hold the
temporaries of the addition and access those from two threads at the
same time. Then you deserve every race condition there is. I don't
think there is code like this, and if there is, it is clearly wrong and
violating layers and interfaces and common sense.
|
|
Out of the top of my head... not recalling any specific add/sub instance. The ++ and -- instances that got changed to However, the split brain syndrome is present many places where an input pin or param is read multiple times and is changed from userspace in the meantime (there is no lock preventing it). Several of them got fixed by proper caching (for example deadband in joyhandle, div2 and invert). Many, many more places are still reading the same thing multiple times. It is both slow (due to the volatility) and a potential value race. |
Since the move to 64-bit HAL types every integer pin is 64 bits wide. On 32-bit targets the plain volatile accessors compile to two 32-bit accesses (two movl on i386), so a reader in another thread can see half of a write: at a zero crossing the value is off by 2^32. A test with one thread toggling a sint pin between -1 and 0 and another reading it sees millions of torn values per second on i386, and still about 30 per second pinned to one CPU. A real path is a userspace jog wheel (e.g. shuttle) writing counts that the servo thread reads as joint.N.jog-counts. Use __atomic_load_n/__atomic_store_n (__atomic_load/__atomic_store for real) with __ATOMIC_RELAXED in the getters and setters. On amd64 and arm64 this compiles to the same single loads and stores as before; on i386 it is fildq/fistpq and on ARMv7 ldrexd/strexd. The same test sees no torn values. This also makes real pins atomic on ARMv7, where vldr/vstr were not guaranteed to be. Without lock-free 64-bit atomics the compiler falls back to libatomic, whose locks are process-local and do not protect HAL shared memory, so configure now fails on such targets (e.g. i486, ARM before ARMv6K), as requested by Bertho Stultiens in LinuxCNC#4614.
|
@rmu75 agreed that atomic load and store are enough with one writer per signal, and
@BsAtHome agreed on the split brain reads. That one is the same on 64-bit, though, and the fix is caching the input once per function, component by component; #4683 neither fixes nor worsens it. |
Since the move to 64-bit HAL types every integer pin is 64 bits wide. On 32-bit targets the plain volatile accessors compile to two 32-bit accesses (two movl on i386), so a reader in another thread can see half of a write: at a zero crossing the value is off by 2^32. A test with one thread toggling a sint pin between -1 and 0 and another reading it sees millions of torn values per second on i386, and still about 30 per second pinned to one CPU. A real path is a userspace jog wheel (e.g. shuttle) writing counts that the servo thread reads as joint.N.jog-counts. Use __atomic_load_n/__atomic_store_n (__atomic_load/__atomic_store for real) with __ATOMIC_RELAXED in the getters and setters. On amd64 and arm64 this compiles to the same single loads and stores as before; on i386 it is fildq/fistpq and on ARMv7 ldrexd/strexd. The same test sees no torn values. This also makes real pins atomic on ARMv7, where vldr/vstr were not guaranteed to be. Without lock-free 64-bit atomics the compiler falls back to libatomic, whose locks are process-local and do not protect HAL shared memory, so configure now fails on such targets (e.g. i486, ARM before ARMv6K), as requested by Bertho Stultiens in LinuxCNC#4614.
4 participants
libnml: fix the 32-bit build
Prompted by the 32-bit discussion in #4611, I ran an i386 build (32-bit Debian trixie container on x86-64) to see what dropping 32-bit userspace would actually buy. Answer: not much, because exactly one thing is broken.
What broke
Commit efe4ef2 replaced the
CMS_UPDATER::update(long int &)overloads with fixed-widthint64_tones. On LP64int64_tislong, so all the callers passinglongstill compiled and nobody noticed. On ILP32 (i386, armhf)int64_tislong long,longmatches no overload, and the build fails with 260 errors. All of them are inlibnml(cms.cc,nml.cc); the rest of the tree compiles clean on i386.The affected declarations are all from 2004 and were fine as long as a
longoverload existed.Fix
Change the declarations that feed
update()fromlongto explicitint64_t:CMS_HEADERandCMS_QUEUING_HEADERfields, theNMLmsgsize member and constructors,RCS_CMD_MSGconstructor, two locals innml.cc, and thecms_inbuffer_header_sizepointer.18 lines in 7 files. On LP64 this is the same type as before, so no functional change on 64-bit.
Verification
Note independent of 32-bit support
Whether or not 32-bit userspace stays supported, this change is worth having: with
long, the NML encoded header layout depended on the architecture'slongwidth, so mixed-arch NML peers disagreed on the format. With explicitint64_tthe layout is the same everywhere.