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. |
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.