Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
In article <5912ca9e-b4e7-423d-a45d-f4693d1c9...@zoulas.com>,
Christos Zoulas wrote:
>-=-=-=-=-=-
Here's the final changes
- Make ALIGNED_POINTER use __alignof(t) instead of sizeof(t). This is more
correct because it works with non-primitive types and provides the ABI
alignment for the type the compiler will use.
- Remove all the *_HDR_ALIGNMENT macros and asserts
- Replace POINTER_ALIGNED_P with ACCESSIBLE_POINTER which is identical to
ALIGNED_POINTER, but returns that the pointer is always aligned if the
CPU supports unaligned accesses.
Index: sys/mbuf.h
===
RCS file: /cvsroot/src/sys/sys/mbuf.h,v
retrieving revision 1.231
diff -u -u -r1.231 mbuf.h
--- sys/mbuf.h 17 Feb 2021 22:32:04 - 1.231
+++ sys/mbuf.h 17 Feb 2021 23:54:31 -
@@ -843,15 +843,21 @@
m->m_pkthdr.rcvif_index = n->m_pkthdr.rcvif_index;
}
+#define M_GET_ALIGNED_HDR(m, type, linkhdr) \
+m_get_aligned_hdr((m), __alignof(type) - 1, sizeof(type), (linkhdr))
+
static __inline int
-m_get_aligned_hdr(struct mbuf **m, int align, size_t hlen, bool linkhdr)
+m_get_aligned_hdr(struct mbuf **m, int mask, size_t hlen, bool linkhdr)
{
- if (POINTER_ALIGNED_P(mtod(*m, void *), align) == 0) {
- --align;// Turn into mask
+#ifndef __NO_STRICT_ALIGNMENT
+ if (((uintptr_t)mtod(*m, void *) & mask) != 0)
*m = m_copyup(*m, hlen,
- linkhdr ? (max_linkhdr + align) & ~align : 0);
- } else if (__predict_false((size_t)(*m)->m_len < hlen))
+ linkhdr ? (max_linkhdr + mask) & ~mask : 0);
+ else
+#endif
+ if (__predict_false((size_t)(*m)->m_len < hlen))
*m = m_pullup(*m, hlen);
+
return *m == NULL;
}
Index: sys/param.h
===
RCS file: /cvsroot/src/sys/sys/param.h,v
retrieving revision 1.689
diff -u -u -r1.689 param.h
--- sys/param.h 17 Feb 2021 22:32:04 - 1.689
+++ sys/param.h 17 Feb 2021 23:54:31 -
@@ -281,16 +281,24 @@
#defineALIGN(p)(((uintptr_t)(p) + ALIGNBYTES) &
~ALIGNBYTES)
#endif
#ifndef ALIGNED_POINTER
-#defineALIGNED_POINTER(p,t)uintptr_t)(p)) & (sizeof(t) - 1))
== 0)
+#defineALIGNED_POINTER(p,t)uintptr_t)(p)) & (__alignof(t) -
1)) == 0)
#endif
#ifndef ALIGNED_POINTER_LOAD
#defineALIGNED_POINTER_LOAD(q,p,t) (*(q) = *((const t *)(p)))
#endif
+/*
+ * Return if the pointer p is accessible for type t. For primitive types
+ * this means that the pointer itself can be dereferenced; for structures
+ * and unions this means that any field can be dereferenced. On CPUs
+ * that allow unaligned pointer access, we always return that the pointer
+ * is accessible to prevent unnecessary copies, although this might not be
+ * necessarily faster.
+ */
#ifdef __NO_STRICT_ALIGNMENT
-#definePOINTER_ALIGNED_P(p, a) 1
+#defineACCESSIBLE_POINTER(p, t)1
#else
-#definePOINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1))
== 0)
+#defineACCESSIBLE_POINTER(p, t)ALIGNED_POINTER(p, t)
#endif
/*
Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.42
diff -u -u -r1.42 if_arp.h
--- net/if_arp.h17 Feb 2021 22:32:04 - 1.42
+++ net/if_arp.h17 Feb 2021 23:54:31 -
@@ -72,8 +72,6 @@
uint8_t ar_tpa[]; /* target protocol address */
#endif
};
-#defineARP_HDR_ALIGNMENT __alignof(struct arphdr)
-__CTASSERT(ARP_HDR_ALIGNMENT == 2);
static __inline uint8_t *
ar_data(struct arphdr *ap)
Index: net/if_bridge.c
===
RCS file: /cvsroot/src/sys/net/if_bridge.c,v
retrieving revision 1.178
diff -u -u -r1.178 if_bridge.c
--- net/if_bridge.c 14 Feb 2021 20:58:34 - 1.178
+++ net/if_bridge.c 17 Feb 2021 23:54:31 -
@@ -2806,7 +2806,7 @@
if (*mp == NULL)
return -1;
- if (m_get_aligned_hdr(&m, IP_HDR_ALIGNMENT, sizeof(*ip), true) != 0) {
+ if (M_GET_ALIGNED_HDR(&m, struct ip, true) != 0) {
/* XXXJRT new stat, please */
ip_statinc(IP_STAT_TOOSMALL);
goto bad;
@@ -2900,7 +2900,7 @@
* it. Otherwise, if it is aligned, make sure the entire base
* IPv6 header is in the first mbuf of the chain.
*/
- if (m_get_aligned_hdr(&m, IP6_HDR_ALIGNMENT, sizeof(*ip6), true) != 0) {
+ if (M_GET_ALIGNED_HDR(&m, struct ip6_hdr, true) != 0) {
struct ifnet *inifp = m_get_rcvif_NOMPSAFE(m);
/* XXXJRT new stat, please */
ip6_statinc(IP6_STAT_TOOSMALL);
Index: netinet/if_arp.c
===
RCS file: /cvsroot/src/sys/n
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
> On Feb 17, 2021, at 4:20 PM, Valery Ushakov wrote: > > On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote: > > This incrementally improves wrong things b/c you still have the "is > aligned" and "ought to be aligned" conflated in one. The incremental patch improves things and does not make things worse. I will address the __NO_STRICT_ALIGNMENT issue separately. I am planning to propose something in tech-kern once I have it working to my liking. This patch helps me because it aligns the semantics of the two macros better. christos signature.asc Description: Message signed with OpenPGP
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote: > In article , > Valery Ushakov wrote: > > >But to get back to my main point, PLEASE, can we stop making random > >aimless changes without prior discussion? > > Here's the change I'd like to make: > - pass the alignment instead of the mask (as Roy asked and to match the > other macro) > - use alignof to determine that alignment and CTASSERT what we expect > - remove unused macros > > This incrementally improves things. [...] > diff -u -p -u -r1.688 param.h > --- sys/param.h 15 Feb 2021 19:46:53 - 1.688 > +++ sys/param.h 17 Feb 2021 17:45:55 - > @@ -290,7 +290,7 @@ > #ifdef __NO_STRICT_ALIGNMENT > #define POINTER_ALIGNED_P(p, a) 1 > #else > -#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & (a)) == 0) > +#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) > == 0) > #endif > > /* This incrementally improves wrong things b/c you still have the "is aligned" and "ought to be aligned" conflated in one. -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
In article ,
Valery Ushakov wrote:
>But to get back to my main point, PLEASE, can we stop making random
>aimless changes without prior discussion?
Here's the change I'd like to make:
- pass the alignment instead of the mask (as Roy asked and to match the
other macro)
- use alignof to determine that alignment and CTASSERT what we expect
- remove unused macros
This incrementally improves things.
christos
Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.41
diff -u -p -u -r1.41 if_arp.h
--- net/if_arp.h16 Feb 2021 10:20:56 - 1.41
+++ net/if_arp.h17 Feb 2021 17:45:55 -
@@ -72,7 +72,8 @@ structarphdr {
uint8_t ar_tpa[]; /* target protocol address */
#endif
};
-#defineARP_HDR_ALIGNMENT 1
+#defineARP_HDR_ALIGNMENT __alignof(struct arphdr)
+__CTASSERT(ARP_HDR_ALIGNMENT == 2);
static __inline uint8_t *
ar_data(struct arphdr *ap)
Index: netinet/icmp_private.h
===
RCS file: /cvsroot/src/sys/netinet/icmp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 icmp_private.h
--- netinet/icmp_private.h 14 Feb 2021 20:58:35 - 1.4
+++ netinet/icmp_private.h 17 Feb 2021 17:45:55 -
@@ -44,7 +44,6 @@ extern percpu_t *icmpstat_percpu;
#defineICMP_STATINC(x) _NET_STATINC(icmpstat_percpu, x)
-#defineICMP_HDR_ALIGNMENT 3
#endif /* _KERNEL_ */
#endif /* !_NETINET_ICMP_PRIVATE_H_ */
Index: netinet/igmp_var.h
===
RCS file: /cvsroot/src/sys/netinet/igmp_var.h,v
retrieving revision 1.26
diff -u -p -u -r1.26 igmp_var.h
--- netinet/igmp_var.h 14 Feb 2021 20:58:35 - 1.26
+++ netinet/igmp_var.h 17 Feb 2021 17:45:55 -
@@ -105,8 +105,6 @@
*/
#defineIGMP_RANDOM_DELAY(X)(cprng_fast32() % (X) + 1)
-#defineIGMP_HDR_ALIGNMENT 3
-
void igmp_init(void);
void igmp_input(struct mbuf *, int, int);
intigmp_joingroup(struct in_multi *);
Index: netinet/ip_private.h
===
RCS file: /cvsroot/src/sys/netinet/ip_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 ip_private.h
--- netinet/ip_private.h14 Feb 2021 20:58:35 - 1.4
+++ netinet/ip_private.h17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *ipstat_percpu;
#defineIP_STATINC(x) _NET_STATINC(ipstat_percpu, x)
#defineIP_STATDEC(x) _NET_STATDEC(ipstat_percpu, x)
-#defineIP_HDR_ALIGNMENT3
+#defineIP_HDR_ALIGNMENT__alignof(struct ip)
+__CTASSERT(IP_HDR_ALIGNMENT == 4);
#endif /* _KERNEL */
#endif /* !_NETINET_IP_PRIVATE_H_ */
Index: netinet/tcp_private.h
===
RCS file: /cvsroot/src/sys/netinet/tcp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 tcp_private.h
--- netinet/tcp_private.h 14 Feb 2021 20:58:35 - 1.4
+++ netinet/tcp_private.h 17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *tcpstat_percpu;
#defineTCP_STATINC(x) _NET_STATINC(tcpstat_percpu, x)
#defineTCP_STATADD(x, v) _NET_STATADD(tcpstat_percpu, x, v)
-#defineTCP_HDR_ALIGNMENT 3
+#defineTCP_HDR_ALIGNMENT __alignof(struct tcphdr)
+__CTASSERT(TCP_HDR_ALIGNMENT == 4);
#endif /* _KERNEL */
#endif /* !_NETINET_TCP_PRIVATE_H_ */
Index: netinet/udp_private.h
===
RCS file: /cvsroot/src/sys/netinet/udp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 udp_private.h
--- netinet/udp_private.h 14 Feb 2021 20:58:35 - 1.4
+++ netinet/udp_private.h 17 Feb 2021 17:45:55 -
@@ -39,7 +39,8 @@ externpercpu_t *udpstat_percpu;
#defineUDP_STATINC(x) _NET_STATINC(udpstat_percpu, x)
-#defineUDP_HDR_ALIGNMENT 3
+#defineUDP_HDR_ALIGNMENT __alignof(struct udphdr)
+__CTASSERT(UDP_HDR_ALIGNMENT == 2);
#endif /* _KERNEL */
#endif /* !_NETINET_UDP_PRIVATE_H_ */
Index: netinet6/ip6_private.h
===
RCS file: /cvsroot/src/sys/netinet6/ip6_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 ip6_private.h
--- netinet6/ip6_private.h 14 Feb 2021 20:58:35 - 1.4
+++ netinet6/ip6_private.h 17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *ip6stat_percpu;
#defineIP6_STATINC(x) _NET_STATINC(ip6stat_percpu, x)
#defineIP6_STATDEC(x) _NET_STATDEC(ip6stat_percpu, x)
-#defineIP6_HDR_ALIGNMENT 3
+#defineIP6_HDR_ALIGNMENT __alignof(struct ip6_hdr)
+__CTASSERT(IP6_HDR_ALIGNMENT == 4);
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 18:51:43 -0500, Christos Zoulas wrote:
> On Feb 17, 2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
> -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
>
> | Well, it was you who did the definion of ALIGNED_POINTER centralized
> | and overridable :)
> |
> | revision 1.400
> | date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines:
> +26 -1;
> | Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
> | and avoid definining them in 10 different places if not needed.
>
> If you look a bit deeper into that change, it merged many MD copies
> of this macro into one, and I needed the override for the archs that
> were different.
>
> | ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly
> | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
> | That is most likely an oversight, but that will probably require some
> | cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside
> | an __NO_STRICT_ALIGNMENT #ifdef.
>
> This is a mess as you can see. The problem is that in each case we
> need to determine if this macro is used to test alignment to determine
> access restrictions on certain architectures (most cases), or it
> is done for performance/protocol requirements.
>
> I hope that nothing falls into the second category and then we can
> use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
> and __alignof() which my guess is that it did not exist at the time
> the macro was invented, and thus it used sizeof() and limited it to
> integral types.
>
> | We don't even seem to be sure about its semantics, as far as I can
> | tell (see bus space comments in my mail).
> |
> | That's even more of a reason to stop doing aimless random changes
> | without getting some kind of understanding first. The last thing we
> | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
> | different semantics both of which are counter-intuitive to begin with
> | (and riastradh@ even had to add a verbose comment for that).
>
> This change was not aimless. I wanted to remove the multiple copies of
> the m_copyup/m_pullup code into one function. To do that I needed the
> alignment as a value, not a predicate macro (that was again copied in
> ~10 places). When I tried to centralize it, I wanted to do the minimal
> change so I would not screw it up (and I did end up screwing it up).
>
> This is a good opportunity now to clean this up even more and provide
> sane alignment macros.
We should start by at least separating the concerns. The test for
"aligned at power of two boundary" and the actual intent/purpose of
that test ("stuff needs to be aligned for us to safely do $FOO").
Then we can test the alignment check without jumping through
ridiculous hoops. That should have been done earlier for the
ALIGNED_POINTER change, but yeah, I can see how in the heat of the
moment that change was just "preserve the status quo" and that's
absolutely fine. But doing the same thing now with the alignment test
and the purpose of that alignment test conflacted once again under a
different but confusignly similar name is negligent (if anything we
will run out of half way decent names for the just-the-alignment-test
macro, b/c all of them will be loaded with some additional accidental
semantic).
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Wed, Feb 17, 2021 at 00:51:03 +0100, Roland Illig wrote: > 17.02.2021 00:25:10 Valery Ushakov : > > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote: > >> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the > >> test is for. > >> > >> I intentionally placed it between (which defines that > >> macro on x86 and some other platforms) and (which uses the > >> macro to switch between the boring "everything is correctly aligned" and > >> the more interesting formula suggested here. > > > > This is wrong on so many levels. What is even the point of a test > > that doesn't test the thing as it is actually defined and used in the > > code? > > The point of the test is to verify that the "complicated" formula > produces correct results. That's what several commits tried to > fix. If this test had been there from the beginning, none of the > wrong formulas would have passed it. That's the whole point. > > The point of the test was intentionally not to test the actual > behavior on each platform but to test the same formula, independent > from the platform, and to do this, I somehow needed access to that > formula. Testing the actually used formula per platform could be > added as another test. I just wanted to avoid the obviously wrong > formulas to go unnoticed in the code. That's the point of the test, > and that's exactly what it achieves. Therefore I don't see anything > wrong with it. The very fact that you need to undefine an unspecified macro at an unspecified time to get to the "formula" points to a problem. We shouldn't be pretending that it's not, and provide the false decorum of "oh, but it's covered with a test, so it's ok". -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Feb 17, 2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote: -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys) | Well, it was you who did the definion of ALIGNED_POINTER centralized | and overridable :) | | revision 1.400 | date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines: +26 -1; | Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently, | and avoid definining them in 10 different places if not needed. If you look a bit deeper into that change, it merged many MD copies of this macro into one, and I needed the override for the archs that were different. | ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT. | That is most likely an oversight, but that will probably require some | cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside | an __NO_STRICT_ALIGNMENT #ifdef. This is a mess as you can see. The problem is that in each case we need to determine if this macro is used to test alignment to determine access restrictions on certain architectures (most cases), or it is done for performance/protocol requirements. I hope that nothing falls into the second category and then we can use a single macro that uses a combination of __NO_STRICT_ALIGNMENT and __alignof() which my guess is that it did not exist at the time the macro was invented, and thus it used sizeof() and limited it to integral types. | We don't even seem to be sure about its semantics, as far as I can | tell (see bus space comments in my mail). | | That's even more of a reason to stop doing aimless random changes | without getting some kind of understanding first. The last thing we | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly | different semantics both of which are counter-intuitive to begin with | (and riastradh@ even had to add a verbose comment for that). This change was not aimless. I wanted to remove the multiple copies of the m_copyup/m_pullup code into one function. To do that I needed the alignment as a value, not a predicate macro (that was again copied in ~10 places). When I tried to centralize it, I wanted to do the minimal change so I would not screw it up (and I did end up screwing it up). This is a good opportunity now to clean this up even more and provide sane alignment macros. christos
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
17.02.2021 00:25:10 Valery Ushakov : > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote: >> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the >> test is for. >> >> I intentionally placed it between (which defines that >> macro on x86 and some other platforms) and (which uses the >> macro to switch between the boring "everything is correctly aligned" and >> the more interesting formula suggested here. > > This is wrong on so many levels. What is even the point of a test > that doesn't test the thing as it is actually defined and used in the > code? The point of the test is to verify that the "complicated" formula produces correct results. That's what several commits tried to fix. If this test had been there from the beginning, none of the wrong formulas would have passed it. That's the whole point. The point of the test was intentionally not to test the actual behavior on each platform but to test the same formula, independent from the platform, and to do this, I somehow needed access to that formula. Testing the actually used formula per platform could be added as another test. I just wanted to avoid the obviously wrong formulas to go unnoticed in the code. That's the point of the test, and that's exactly what it achieves. Therefore I don't see anything wrong with it. Roland
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On 16.02.2021 23:32, Jason Thorpe wrote: On Feb 16, 2021, at 1:56 PM, Roland Illig wrote: A downside of this test is that the macro from gets only evaluated at compile time of the test. The test therefore cannot cover future updates to without being reinstalled itself. But at least for the release builds, it ensures a correct definition. You can make this get evaluated at run-time by passing in a non-constant argument. I wanted to make the test even more flexible: react to the then-current installed version of the macro in . To do that, the test would need to be compiled at runtime, requiring a C compiler on the target machine, and I'm not sure that's a valid assumption. And maybe that's something that shouldn't be done at all, updating the system header without the corresponding test suite. So it's probably fine as it is now, notwithstanding the discussion about whether the macro is needed at all in this prominent place. It surprised me though that neither Google nor GitHub found the macro name POINTER_ALIGNED_P anywhere outside the NetBSD repository. I would have expected this name to be already used by some projects, given that it is so straight-forward. Roland
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 17:53:09 -0500, Christos Zoulas wrote: > In this case "type" is a struct and we have __alignof() to handle > it, but does this give the right answer? > > Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and > can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all > for having one macro, but how can we satisfy all the different > semantics? Well, it was you who did the definion of ALIGNED_POINTER centralized and overridable :) revision 1.400 date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines: +26 -1; Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently, and avoid definining them in 10 different places if not needed. ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT. That is most likely an oversight, but that will probably require some cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside an __NO_STRICT_ALIGNMENT #ifdef. We don't even seem to be sure about its semantics, as far as I can tell (see bus space comments in my mail). That's even more of a reason to stop doing aimless random changes without getting some kind of understanding first. The last thing we need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly different semantics both of which are counter-intuitive to begin with (and riastradh@ even had to add a verbose comment for that). -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
> On 16.02.2021 23:40, Valery Ushakov wrote:
> > On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> >
> > > On 16.02.2021 19:46, Roy Marples wrote:
> > > > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > > > want rather than the bitwise test we supply, like so:
> > > >
> > > > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> > >
> > > To make sure that this macro doesn't get broken again, I have written
> > > the attached tests. I will commit them after making sure I got the
> > > changes to distrib/sets/lists/tests/mi correct. All the rest I already
> > > tested successfully.
> >
> > I don't see any proposal to change the meaning of this macro to
> > actually require the alignment even for arches without strict
> > alignment. Does the attached test really passes on e.g. x86 where the
> > macro is always true? E.g. this one:
> >
> > > + if (POINTER_ALIGNED_P(p + 1, 2))
> > > + atf_tc_fail("p + 1 must not be aligned to 2");
>
> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> test is for.
>
> I intentionally placed it between (which defines that
> macro on x86 and some other platforms) and (which uses the
> macro to switch between the boring "everything is correctly aligned" and
> the more interesting formula suggested here.
This is wrong on so many levels. What is even the point of a test
that doesn't test the thing as it is actually defined and used in the
code?
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On 16.02.2021 23:40, Valery Ushakov wrote:
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
On 16.02.2021 19:46, Roy Marples wrote:
I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:
#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
To make sure that this macro doesn't get broken again, I have written
the attached tests. I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct. All the rest I already
tested successfully.
I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment. Does the attached test really passes on e.g. x86 where the
macro is always true? E.g. this one:
+ if (POINTER_ALIGNED_P(p + 1, 2))
+ atf_tc_fail("p + 1 must not be aligned to 2");
Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the
test is for.
I intentionally placed it between (which defines that
macro on x86 and some other platforms) and (which uses the
macro to switch between the boring "everything is correctly aligned" and
the more interesting formula suggested here.
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
In this case "type" is a struct and we have __alignof() to handle it, but does this give the right answer? Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all for having one macro, but how can we satisfy all the different semantics? christos signature.asc Description: Message signed with OpenPGP
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> On 16.02.2021 19:46, Roy Marples wrote:
> > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > want rather than the bitwise test we supply, like so:
> >
> > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
>
> That's a good definition, clear, simple, obvious, without any surprises.
Now, replace the value "a" with the type "t" and change (a) to
sizeof(t) and you will get ALIGNED_POINTER from just above.
> To make sure that this macro doesn't get broken again, I have written
> the attached tests. I will commit them after making sure I got the
> changes to distrib/sets/lists/tests/mi correct. All the rest I already
> tested successfully.
I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment. Does the attached test really passes on e.g. x86 where the
macro is always true? E.g. this one:
> + if (POINTER_ALIGNED_P(p + 1, 2))
> + atf_tc_fail("p + 1 must not be aligned to 2");
> Is my assumption correct that on each platform supported by NetBSD, a
> variable of type double gets aligned to a multiple of 8, even on the
> stack?
No. E.g. on sh3 doubles are 8 bytes but are 4 bytes aligned. I'm
almost sure some other ABI has that kind less strict alignment too,
but I don't remember.
> I wanted to keep the test as simple as possible, therefore I
> didn't want to call malloc just to get an aligned pointer.
You can use an ordinary array that is large enough and manually
find/construct a suitably aligned pointer value inside that array.
BUT, can we, PLEASE, stop making random breaking changes and at least
articulate first what is that that we want here?
POINTER_ALIGNED_P should have never been brought into existence in the
first place.
ALIGNED_POINTER seems to be used to define BUS_SPACE_ALIGNED_POINTER -
the real fun here is sys/arch/x86/include/bus_defs.h that defines
BUS_SPACE_ALIGNED_POINTER to be "really" aligned for BUS_SPACE_DEBUG,
which seems kinda suspicious to me. BTW, can we really conclude from
the CPU's alignment requirements to some bus alignment requirements?
But to get back to my main point, PLEASE, can we stop making random
aimless changes without prior discussion?
To quote Vonnegut, "If I had of been a observer, I would of said we
was comical."
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
> On Feb 16, 2021, at 1:56 PM, Roland Illig wrote: > > A downside of this test is that the macro from gets only > evaluated at compile time of the test. The test therefore cannot cover > future updates to without being reinstalled itself. But > at least for the release builds, it ensures a correct definition. You can make this get evaluated at run-time by passing in a non-constant argument. -- thorpej
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On 16.02.2021 19:46, Roy Marples wrote:
I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:
#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1))
That's a good definition, clear, simple, obvious, without any surprises.
To make sure that this macro doesn't get broken again, I have written
the attached tests. I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct. All the rest I already
tested successfully.
Is my assumption correct that on each platform supported by NetBSD, a
variable of type double gets aligned to a multiple of 8, even on the
stack? I wanted to keep the test as simple as possible, therefore I
didn't want to call malloc just to get an aligned pointer.
A downside of this test is that the macro from gets only
evaluated at compile time of the test. The test therefore cannot cover
future updates to without being reinstalled itself. But
at least for the release builds, it ensures a correct definition.
Roland
Index: distrib/sets/lists/tests/mi
===
RCS file: /cvsroot/src/distrib/sets/lists/tests/mi,v
retrieving revision 1.1019
diff -u -p -r1.1019 mi
--- distrib/sets/lists/tests/mi 16 Feb 2021 09:46:24 - 1.1019
+++ distrib/sets/lists/tests/mi 16 Feb 2021 21:45:40 -
@@ -4396,6 +4396,10 @@
./usr/tests/sys/rc/h_args tests-sys-tests
compattestfile,atf
./usr/tests/sys/rc/h_simpletests-sys-tests
compattestfile,atf
./usr/tests/sys/rc/t_rc_d_cli tests-sys-tests
compattestfile,atf
+./usr/tests/sys/systests-sys-tests
compattestfile,atf
+./usr/tests/sys/sys/Atffiletests-sys-tests
compattestfile,atf
+./usr/tests/sys/sys/Kyuafile tests-sys-tests
compattestfile,atf,kyua
+./usr/tests/sys/sys/t_pointer_aligned_ptests-sys-tests
compattestfile,atf
./usr/tests/sys/x86tests-sys-tests
compattestfile,atf
./usr/tests/syscalltests-obsolete
obsolete
./usr/tests/syscall/Atffiletests-obsolete
obsolete
Index: tests/sys/Makefile
===
RCS file: /cvsroot/src/tests/sys/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- tests/sys/Makefile 15 Oct 2020 17:44:44 - 1.5
+++ tests/sys/Makefile 16 Feb 2021 21:45:40 -
@@ -10,6 +10,7 @@ TESTS_SUBDIRS+= netatalk
TESTS_SUBDIRS+=netinet
TESTS_SUBDIRS+=netinet6
TESTS_SUBDIRS+=rc
+TESTS_SUBDIRS+=sys
.if ${MACHINE} == amd64 || ${MACHINE} == i386
TESTS_SUBDIRS+=x86
.endif
Index: tests/sys/sys/Makefile
===
RCS file: tests/sys/sys/Makefile
diff -N tests/sys/sys/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ tests/sys/sys/Makefile 16 Feb 2021 21:45:40 -
@@ -0,0 +1,11 @@
+# $NetBSD$
+
+TESTSDIR= ${TESTSBASE}/sys/sys
+WARNS= 6
+
+TESTS_C= t_pointer_aligned_p
+
+.PATH: ${NETBSDSRCDIR}/sys
+CPPFLAGS+= -I${NETBSDSRCDIR}/sys
+
+.include
Index: tests/sys/sys/t_pointer_aligned_p.c
===
RCS file: tests/sys/sys/t_pointer_aligned_p.c
diff -N tests/sys/sys/t_pointer_aligned_p.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ tests/sys/sys/t_pointer_aligned_p.c 16 Feb 2021 21:45:40 -
@@ -0,0 +1,77 @@
+/* $NetBSD$*/
+
+/*-
+ * Copyright (c) 2021 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON
