mdns: Fixed build on big-endian ports

[tinic]

nxd_mdns.c uses NX_CHANGE_USHORT_ENDIAN() as an expression:

*(USHORT *)(packet_ptr -> nx_packet_prepend_ptr + NX_MDNS_FLAGS_OFFSET)
    |= NX_CHANGE_USHORT_ENDIAN(tc_bit);

Big-endian ports define that macro as empty, so it expands to "*(USHORT *)(...) |= ;" and addons/mdns fails to compile on every big-endian target.

It goes unnoticed on little-endian because those ports define the macro as an assignment -- a = (((a >> 8) | (a << 8)) & 0xFFFF) -- which has a value and so parses in expression position. Every other call site in the repository uses the macro as a statement, which is what an unconditionally-empty definition requires; see for example test/regression/ptp_test/netx_ptp_utility.c. This is the only expression use.

Swap in place, then OR, which keeps the macro used as a statement.

Verified against both upstream definitions: with the big-endian (empty) definition the original fails to compile and this compiles; with the little-endian definition both produce the same flags word, 0x0002.

Found while building addons/mdns for m68k AmigaOS.

[fdesbiens]

Thank you — this is a tidy diagnosis and the fix is the right shape. I verified all of it rather than the premise alone, since "fixes the build on big-endian" is a claim worth testing.

The macro definitions are as you describe. ports/linux/gnu/inc/nx_port.h:96 defines it as an assignment under #ifdef NX_LITTLE_ENDIAN, and :123 defines it as empty in the #else. So the little-endian form has a value and parses in expression position, and the big-endian form does not.

It really is the only expression use. I scanned every .c and .h in the tree for NX_CHANGE_USHORT_ENDIAN and NX_CHANGE_ULONG_ENDIAN and classified each occurrence by whether anything other than whitespace precedes it on the line. Exactly one non-statement use, and it is the line you are fixing:

non-statement (expression) uses of the endian macros:
  addons/mdns/nxd_mdns.c:8489
total: 1

Both behaviours confirmed by compiling. I built the two upstream definitions against the original and patched forms:

  little-endian defn | original : compiles, flags word = 0x0002
  little-endian defn | PR fix   : compiles, flags word = 0x0002
  big-endian defn    | original : COMPILE ERROR -> expected expression before ';' token
  big-endian defn    | PR fix   : compiles, flags word = 0x0200

So no behavioural change on little-endian, and the big-endian result is 0x0200 — which is right, since on a big-endian host the TC flag is already in network order and no swap is wanted. The fix does not merely make it compile; it produces the correct value on the platform that could not build.

And against the real file. I emulated a big-endian port by taking ports/linux/gnu/inc/nx_port.h with NX_LITTLE_ENDIAN commented out, then compiled addons/mdns/nxd_mdns.c:

• on dev: nxd_mdns.c:8489:116: error: expected expression before ';' token
• with this PR: compiles clean

addons/mdns contains exactly one .c file, so that single change does fix the whole add-on and your title is accurate rather than optimistic. I also rebuilt it little-endian under -Werror -Wall -Wextra and it is clean, so there is no regression.

One detail that makes the in-place swap safe, worth recording because it is the thing that could have made this wrong: tc_bit is declared at :8365, assigned at :8488 and used at :8489, and never read again. Swapping it in place therefore cannot affect anything downstream. Both forms mutate it in any case — the little-endian macro is an assignment, so the original mutated it too and merely also yielded the value.

Incidentally the patch also drops a stray extra leading space that the original line carried, so the indentation now matches its neighbours. Not worth mentioning except that it makes the diff one character wider than it looks.

The two notes below are about prevention rather than this change, and I would not hold the PR for either.

---

1. Follow-up — the macro permits on one platform what it forbids on the other

Anchor: addons/mdns/nxd_mdns.c line 8489 (NX_CHANGE_USHORT_ENDIAN(tc_bit);)

Your change is correct and I would merge it as-is. This note is about the shape of the macro, which is what allowed the mistake and will allow the next one.

The two definitions disagree about what kind of construct the macro is. The little-endian form is an assignment expression, so it is usable both as a statement and inside a larger expression. The big-endian form is empty, so it is only usable as a statement. That asymmetry means the platform everybody builds on silently accepts a usage that the platform nobody builds on rejects — which is precisely how this reached six years of releases.

Making both definitions statement-only would move the error to the first little-endian compile, where someone would see it immediately:

/* little-endian */
#define NX_CHANGE_USHORT_ENDIAN(a)  do { a = ((USHORT)((a >> 8) | (a << 8)) & 0xFFFF); } while(0)

/* big-endian */
#define NX_CHANGE_USHORT_ENDIAN(a)  do { } while(0)

That keeps every existing statement use working, including the if (x) NX_CHANGE_...(a); else shape where the idiom matters, and turns any future expression use into a compile error on every port rather than on one.

I am explicitly not asking you to do this here. It touches nx_port.h in all 66 shipped ports, which is a mechanical change of a completely different size from this PR and wants its own review. But it is worth an issue, and you are the right person to have found it.

---

2. Follow-up — nothing in the tree builds big-endian

Anchor: none in this diff.

Worth writing down, because it explains the six years and points at the cheapest prevention.

Every shipped port defines NX_LITTLE_ENDIAN. I checked all of them: the only one that is not a literal #define NX_LITTLE_ENDIAN 1 is ports/mips/gnu/inc/nx_port.h:60, which defines it without a value — and since the guard is #ifdef, that is still little-endian. So the entire big-endian branch of nx_port.h is dead code across the in-tree ports, reachable only from an out-of-tree port. Yours is, as far as I can tell, the first to build it.

That also means there is no in-tree regression risk from this change, which is reassuring, and that CI cannot currently catch this class of defect: .github/workflows/regression_test.yml is the only workflow and it has no cross-compilation or big-endian target.

The cheap prevention is a compile-only job rather than a new port. Building the tree with NX_LITTLE_ENDIAN forced off is enough to catch exactly this failure — it is what I did to reproduce your report, and it is a single gcc -c per file with a doctored port header. No linking, no execution, no target hardware, and it would have failed in 2020.

I will raise that with the rest of the maintainers. Mentioning it here so the reasoning is attached to the change that prompted it.

[fdesbiens]

I will merge this to dev soon, once I have reviewed the current batch of PRs. This will ship with our Q3 release in September.