libnml: use int64_t instead of long for CMS/NML sizes and ids - #4614
grandixximo wants to merge 2 commits into
Conversation
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.
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. |
The thread time/tmax and plasmac low_cut_volts prints used %ld with hal_get_sint() values (int64_t), which only matches on LP64. Use fmt::format for the halcmd thread print (fmt is already used in this file and formats int64_t natively) and a (long long) cast with %lld for the plasmac realtime component, where rtapi_print_msg is the only output channel.
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. |
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.