[Dnsmasq-discuss] v2.91test1 compile failure on FreeBSD

[Matthias Andree]

Am 20.12.24 um 22:16 schrieb Simon Kelley:
>
>
> On 12/20/24 12:27, Matthias Andree via Dnsmasq-discuss wrote:
>> Simon,
>>
>> I cannot compile v2.91test1 on FreeBSD 14.2, errors below. (Neither
>> tarball nor Git compile.) (2nd to last shown errors). Patch attached,
>> should be fine with git-am.
>
> Patch applied, thanks.
>>
>> Adding multiple unions with variable size into some other aggregate
>> (union or struct) isn't portable, first shown warning below, might be
>> an error on other or strictly set compilers. This needs fixing not
>> covered by my patch.
>
> If I've understood this right, then
>
> https://thekelleys.org.uk/gitweb/?
> p=dnsmasq.git;a=commit;h=4902807879c9b39db46c32e3e082a33266836d24
>
> should fix it.

Thanks, but it's not sufficient, and my wording was unclear, sorry for
that.

I am attaching an incremental git-am ready patch to go on top your Git HEAD,
to fix all sorts of issues and make this conforming C99 with default
options set,
and fix another load of warnings you receive when setting the compiler
to pick the nits,
-pedantic-errors -std=c99 (or c11, c18, c2x).
It changes many void * to uint8_t * to make the "increment by bytes"
explicit.
You can't do:

void *foo;
// ...
foo += 2.

IMPORTANT: please review the char data[1] in my patch, whether you want
to increase the size.

I see the six header warnings show below for practically all source
files compiled, and regarding the log message:
The warning is from FreeBSD 14.2's clang 18.1.6 compiler, not GCC, which
only shows them under -pedantic (I used GCC 13.3):

The root cause for these warnings appears to be the VLA char data[]
inside union all_addr's struct datablock, see line 342 of src/dnsmasq.h:

>    336    /* for arbitrary RR record small enough to go in addr.
>    337       NOTE: rrblock and rrdata are discriminated by the
> F_KEYTAG bit
>    338       in the cache flags. */
>    339    struct datablock {
>    340      unsigned short rrtype;
>    341      unsigned char datalen; /* also length of SOA in negative
> records. */
>    342      char data[];
>    343    } rrdata;
>    344  };

Declaring this as char data[1] or data[16] or whatever is suitable
should fix the warnings. data[0] is non-conforming and won't work either.
I don't know what's a good size here, as I haven't read enough context
code to judge that. My patch sets it 1 because that's the minimum in
order not to bloat the structure.

Also note that many of the *_addr types you define are
memory-inefficient because you often have short types before pointer
types, which have alignment requirements almost everywhere, and
something like:

> struct {
>     unsigned short rrtype;
>     unsigned short datalen;
>     struct blockdata *rrdata;
>   } rrblock;

will incur 4 bytes of padding between datalen and rrdata so the pointer
is aligned at 64-bit boundaries.  The standing recommendation is to sort
members by descending order of alignment requirement and size. I. e.
pointers and longs first, then ints, then shorts.

For this particular example, you could write:

struct {
     struct blockdata *rrdata;
     unsigned short rrtype;
     unsigned short datalen;
} rrblock;

to fix that.  IMPORTANT: my patch does not address this!


In many cases, size-definite types (such as uint16_t) are a good choice
if you really know the value range, say, from IETF standards and RFCs,
but would require lots of printf-style format adjustments, too.


> ./dnsmasq.h:350:18: warning: field 'addr' with variable sized type
> 'union all_addr' not at the end of a struct or class is a GNU
> extension [-Wgnu-variable-sized-type-not-at-end]
>   350 |   union all_addr addr;
>       |                  ^
> ./dnsmasq.h:416:18: warning: field 'addr' with variable sized type
> 'union all_addr' not at the end of a struct or class is a GNU
> extension [-Wgnu-variable-sized-type-not-at-end]
>   416 |   union all_addr addr;
>       |                  ^
> ./dnsmasq.h:483:18: warning: field 'addr' with variable sized type
> 'union all_addr' not at the end of a struct or class is a GNU
> extension [-Wgnu-variable-sized-type-not-at-end]
>   483 |   union all_addr addr;
>       |                  ^
> ./dnsmasq.h:782:20: warning: field 'dest' with variable sized type
> 'union all_addr' not at the end of a struct or class is a GNU
> extension [-Wgnu-variable-sized-type-not-at-end]
>   782 |     union all_addr dest;
>       |                    ^
> ./dnsmasq.h:787:5: warning: field 'frec_src' with variable sized type
> 'struct frec_src' not at the end of a struct or class is a GNU
> extension [-Wgnu-variable-sized-type-not-at-end]
>   787 |   } frec_src;
>       |     ^
> ./dnsmasq.h:1108:18: warning: field 'source' with variable sized type
> 'union all_addr' not at the end of a struct or class is a GNU
> extension [-Wgnu-variable-sized-type-not-at-end]
>  1108 |   union all_addr source;
>       |                  ^

GCC shows such warnings in stricter modes (for instance, -pedantic),
clang already does with default options.


There is also this char * vs. void * complaint:

> bpf.c: In function 'arp_enumerate':
> bpf.c:94:40: warning: passing argument 2 of 'callback.af_unspec' from
> incompatible pointer type [-Wincompatible-pointer-types]
>    94 |       if (!callback.af_unspec(AF_INET, &sin2->sin_addr,
> LLADDR(sdl), sdl->sdl_alen, parm))
>       |                                        ^~~~~~~~~~~~~~~
>       |                                        |
>       |                                        struct in_addr *
> bpf.c:94:40: note: expected 'char *' but argument is of type 'struct
> in_addr *'

A simple fix for this one is changing the inf (*af_unspec)...
declaration to make void *addrp instead of char *addrp.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-this-compile-under-pedantic-errors-std-c99-c11-.patch
Type: text/x-patch
Size: 9783 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20241222/0603c448/attachment-0001.bin>