[Dnsmasq-discuss] v2.91test1 compile failure on FreeBSD
Simon Kelley
simon at thekelleys.org.uk
Tue Dec 24 11:07:50 UTC 2024
On 22/12/2024 10:44, Matthias Andree via Dnsmasq-discuss wrote:
> 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.
>
Understood. There are few more instances of this in Linux-only code.
I'll see to those too.
> 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.
data[1] will, I think, work fine.
The plan here is to to have a union tag that stores the rrtype and a
small length field and uses the rest of the space in the union to store
RR data when that's small enough. The size of the union is at least 16
bytes, because it can store an IPv6 address, and I don't want it to get
any larger than that.
The line
#define RR_IMDATALEN (sizeof(union all_addr) - offsetof(struct
datablock, data))
calculates how much data will fit and is used in the code. This is
always 13 bytes at the moment. Sadly, the chicken-and-egg problem
prevents the use of that expression in the declaration of data[]
Declaring data[1] and then writing bytes up to data[12] clearly works
whilst struct rrdata is part of a union of size 16, but I wonder if it
will trip over array-overflow checking?
Maybe we should stop being clever,
#define RR_IMDATALEN 13
and declare data[RR_IMDATALEN];
That at least stays away from language definition areas I don't claim to
understand.
>
> 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;
>
The reason for doing this is that it allows code to find the value of
rrtype from a strut rrblock OR struct rrdata all_addr without doing
different code path for each. No space is wasted in rrblock since the
all_addr union is 16 bytes to store an IPv6 address, and the struct as
defined fits that. It's important to have the rrtype at the start of
struct rrdata to maximise the size of the data field.
> 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.
>
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss at lists.thekelleys.org.uk
> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
More information about the Dnsmasq-discuss
mailing list