Hi Tyler,
Tyler Retzlaff, Mar 22, 2024 at 17:56:
i can answer this!
windows toolchains will only require __extension__ qualification on use
of statement expressions, so msvc won't require any use of __extension__
in this patch.
as a general rule of thumb __extension__ is something you may choose to
use for any gcc compiled code that is an extension to standard C and you
intend to use the -pedantic flag (i.e. -std=c11 && -pedantic used together)
Got it, thanks!
> /* Fast path area */
> #define RTE_NODE_CTX_SZ 16
> - alignas(RTE_CACHE_LINE_SIZE) uint8_t ctx[RTE_NODE_CTX_SZ]; /**< Node
Context. */
> + __extension__ alignas(RTE_CACHE_LINE_SIZE) union {
__extension__ should not be on the anonymous union (since they are standard
C11).
anonymous union declaration is actually a type with no name and then a data
field of that type so __rte_aligned is most likely what you want, since
you're using RTE_CACHE_LINE_SIZE we can use __rte_cache_aligned.
union __rte_cache_aligned {
... your union fields ...
};
and i think checkpatches still gives a warning unrelated to alignment
for this but it can be safely ignored. it's the warning about alignment
that we care about and should be fixed.
This passes the C++ header check but it breaks the static_assert I just
added. I believe the alignment is somehow transferred to all union
fields. And since ctx is an array, it makes the whole union .
So before my patch:
/* --- cacheline 3 boundary (192 bytes) --- */
uint8_t ctx[16] __attribute__((__aligned__(64))); /* 192 16 */
uint16_t size; /* 208 2 */
With the anonymous union aligned:
/* --- cacheline 3 boundary (192 bytes) --- */
union {
uint8_t ctx[16]; /* 192 16 */
struct {
void * ctx_ptr; /* 192 8 */
void * ctx_ptr2; /* 200 8 */
}; /* 192 16 */
} __attribute__((__aligned__(64))); /* 192 64 */
/* --- cacheline 4 boundary (256 bytes) --- */
uint16_t size; /* 256 2 */
However, if I remove the explicit align, I get what I expect:
/* --- cacheline 3 boundary (192 bytes) --- */
union {
uint8_t ctx[16]; /* 192 16 */
struct {
void * ctx_ptr; /* 192 8 */
void * ctx_ptr2; /* 200 8 */
}; /* 192 16 */
}; /* 192 16 */
uint16_t size; /* 208 2 */
Is it OK to drop the explicit alignment? This is beyond my C skills :)
> + uint8_t ctx[RTE_NODE_CTX_SZ];
> + /* Convenience aliases to store pointers without complex casting.
*/
> + __extension__ struct {
this is correct/recommended since anonymous structs aren't standard,
with the __extension__ -pedantic won't emit a warning (our intention).
Ack.
> +static_assert(offsetof(struct rte_node, size) - offsetof(struct rte_node,
ctx) == RTE_NODE_CTX_SZ,
> + "The node context anonymous union cannot be larger than RTE_NODE_CTX_SZ");
> +
you should include directly include <stddef.h> in this file for use of offsetof.
you should include directly include <assert.h> in this file for use of the
static_assert.
Will do for v3.
Thanks!
