Cast pointer to uintptr_t to avoid alignment warnings.

[jhb]

Summary

This fixes a -Waddress-of-packed-member error raised by GCC 9.

[afedorov]

tests/sys/netinet/libalias/util.c

112

Just a side note. Most of the networking code uses: (p-> ip_hl << 2)

Perhaps we need an appropriate macro for this.

[donner]

Which architecture causes this problem?

tests/sys/netinet/libalias/util.c

111–112

I'd prefer uint8_t.

char is not well defined.
It might be 32bit long (seen that).

[jhb]

The build fails with GCC on all architectures.

tests/sys/netinet/libalias/util.c

111–112

FreeBSD doesn't work on architectures where char is not a byte. See all the uses of 'caddr_t'. This is the code in udp_input to calculate the pointer:

	ip = mtod(m, struct ip *);
	uh = (struct udphdr *)((caddr_t)ip + iphlen);

In fact, it'd probably be cleaner to write this instead as

struct udphdr *u = (struct udphdr *)((caddr_t)ip + (ip->ip_hl << 2));

to more closely match udp_input. A hlen helper variable for ip->ip_hl << 2 wouldn't be bad either.

[imp]

If char is really 32-bits long, then the uint8_t type must necessarily also be at least 32-bits long. char is the smallest unit you can have in C, absent non-standard extensions (and ignoring the special case of bit fields which aren't a first-class type).

[donner]

In D31941#721219, @imp wrote:

If char is really 32-bits long, then the uint8_t type must necessarily also be at least 32-bits long. char is the smallest unit you can have in C, absent non-standard extensions (and ignoring the special case of bit fields which aren't a first-class type).

A compiler for DSP had this "feature", nevermind. It's just the reason why I prefer "int8_t" over "char".
The proposal from @jhb does please my feeling, so go for it.

[jhb]

• Update to the pattern used in the kernel.

[jhb]

*sigh* Now this causes a -Wcast-align error with clang even though it's the same exact code as used in udp_input() in the kernel. I guess the kernel doesn't build with -Wcast-align perhaps?

[jhb]

• Use uintptr_t since clang13 warns about the char * version.

[donner]

All the solutions do not address the underlying problem: Do we know, that the payload is correctly assigned or not?
If we know, that we should use

alignas(int32_t) void * pstart = &up[p->ip_hl];
struct udphdr *u = pstart;

If not: every attempt to trick the compiler will cause a bus error on the problematic architecture anyway.

[jhb]

In D31941#771955, @donner wrote:

All the solutions do not address the underlying problem: Do we know, that the payload is correctly assigned or not?
If we know, that we should use

alignas(int32_t) void * pstart = &up[p->ip_hl];
struct udphdr *u = pstart;

If not: every attempt to trick the compiler will cause a bus error on the problematic architecture anyway.

udphdr doesn't need 4 byte alignment, just 2 byte. struct ip has an alignment of 2. What makes the compiler unhappy is the abuse of using a uint32_t pointer as a lazy way to multiply hlen by 4. The compiler (correctly) thinks this means that the intermediate up pointer requires 4 byte alignment. One could instead do something rather gross like:

uint16_t *us = (void *)p;
struct udphdr *u = (void *)&(us[p->ip_hl << 1]);

However, this seems quite obscure (using uint16_t to multiply by 2, then << 1 to multiply by 2 again). Rather than abusing pointer math in this way, the kernel's approach of 'header + len' seems more straightforward.

[donner]

In D31941#772286, @jhb wrote:

udphdr doesn't need 4 byte alignment, just 2 byte. struct ip has an alignment of 2. What makes the compiler unhappy is the abuse of using a uint32_t pointer as a lazy way to multiply hlen by 4. The compiler (correctly) thinks this means that the intermediate up pointer requires 4 byte alignment. One could instead do something rather gross like:

How about

typedef header_word_t uint8_t[4];
header_word_t *us = (void *)p;
struct udphdr *u = (void *)&(us[p->ip_hl]);

Or plain and simple:

struct udphdr *u = (void *)((char *)p + 4*(p->ip_hl));

[jhb]

In D31941#772518, @donner wrote:

In D31941#772286, @jhb wrote:

udphdr doesn't need 4 byte alignment, just 2 byte. struct ip has an alignment of 2. What makes the compiler unhappy is the abuse of using a uint32_t pointer as a lazy way to multiply hlen by 4. The compiler (correctly) thinks this means that the intermediate up pointer requires 4 byte alignment. One could instead do something rather gross like:

How about

typedef header_word_t uint8_t[4];
header_word_t *us = (void *)p;
struct udphdr *u = (void *)&(us[p->ip_hl]);

Or plain and simple:

struct udphdr *u = (void *)((char *)p + 4*(p->ip_hl));

I tried this second one earlier (it was the previous version of this review) but it fails because -Wcast-align on clang doesn't like demoting the the alignment of p (2) to a char * (1) hence why the current patch uses uintptr_t.

[imp]

Missed you on IRC, but this is fine. Lots of ways to do this, and this one is clearer than what was there before and appears to me to be well defined.