[PATCH v5 11/14] eal: expand most macros to empty when using MSVC

[Tyler Retzlaff]

On Fri, Apr 14, 2023 at 08:45:17AM +0200, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla at linux.microsoft.com]
> > Sent: Thursday, 13 April 2023 23.26
> > 
> > For now expand a lot of common rte macros empty. The catch here is we
> > need to test that most of the macros do what they should but at the same
> > time they are blocking work needed to bootstrap of the unit tests.
> > 
> > Later we will return and provide (where possible) expansions that work
> > correctly for msvc and where not possible provide some alternate macros
> > to achieve the same outcome.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla at linux.microsoft.com>
> > ---
> >  lib/eal/include/rte_branch_prediction.h |  8 ++++++
> >  lib/eal/include/rte_common.h            | 45
> > +++++++++++++++++++++++++++++++++
> >  lib/eal/include/rte_compat.h            | 20 +++++++++++++++
> >  3 files changed, 73 insertions(+)
> > 
> > diff --git a/lib/eal/include/rte_branch_prediction.h
> > b/lib/eal/include/rte_branch_prediction.h
> > index 0256a9d..d9a0224 100644
> > --- a/lib/eal/include/rte_branch_prediction.h
> > +++ b/lib/eal/include/rte_branch_prediction.h
> > @@ -25,7 +25,11 @@
> >   *
> >   */
> >  #ifndef likely
> > +#ifndef RTE_TOOLCHAIN_MSVC
> >  #define likely(x)	__builtin_expect(!!(x), 1)
> > +#else
> > +#define likely(x)	(x)
> 
> This must be (!!(x)), because x may be non-Boolean, e.g. likely(n & 0x10), and likely() must return Boolean (0 or 1).

yes, you're right. will fix.

> 
> > +#endif
> >  #endif /* likely */
> > 
> >  /**
> > @@ -39,7 +43,11 @@
> >   *
> >   */
> >  #ifndef unlikely
> > +#ifndef RTE_TOOLCHAIN_MSVC
> >  #define unlikely(x)	__builtin_expect(!!(x), 0)
> > +#else
> > +#define unlikely(x)	(x)
> 
> This must also be (!!(x)), for the same reason as above.

ack

> 
> > +#endif
> >  #endif /* unlikely */
> > 
> >  #ifdef __cplusplus
> > diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> > index 2f464e3..1bdaa2d 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -65,7 +65,11 @@
> >  /**
> >   * Force alignment
> >   */
> > +#ifndef RTE_TOOLCHAIN_MSVC
> >  #define __rte_aligned(a) __attribute__((__aligned__(a)))
> > +#else
> > +#define __rte_aligned(a)
> > +#endif
> 
> It should be reviewed that __rte_aligned() is only used for optimization purposes, and is not required for DPDK to function properly.

so to expand on what i have in mind (and explain why i leave it expanded
empty for now)

while msvc has a __declspec for align there is a mismatch between
where gcc and msvc want it placed to control alignment of objects.

msvc support won't be functional in 23.07 because of atomics. so once
we reach the 23.11 cycle (where we can merge c11 changes) it means we
can also use standard _Alignas which can accomplish the same thing
but portably.

full disclosure the catch is i still have to properly locate the <thing>
that does the alignment and some small questions about the expansion and
use of the existing macro.

on the subject of DPDK requiring proper alignment, you're right it
is generally for performance but also for pre-c11 atomics.

one question i have been asking myself is would the community see value
in more compile time assertions / testing of the size and alignment of
structures and offset of structure fields? we have a few key
RTE_BUILD_BUG_ON() assertions but i've discovered they don't offer
comprehensive protection.

> 
> > 
> >  #ifdef RTE_ARCH_STRICT_ALIGN
> >  typedef uint64_t unaligned_uint64_t __rte_aligned(1);
> > @@ -80,16 +84,29 @@
> >  /**
> >   * Force a structure to be packed
> >   */
> > +#ifndef RTE_TOOLCHAIN_MSVC
> >  #define __rte_packed __attribute__((__packed__))
> > +#else
> > +#define __rte_packed
> > +#endif
> 
> Similar comment as for __rte_aligned(); however, I consider it more likely that structure packing is a functional requirement, and not just used for optimization. Based on my experience, it may be used for packing network structures; perhaps not in DPDK itself but maybe in DPDK applications.

so interestingly i've discovered this is kind of a mess and as you note
some places we can't just "fix" it for abi compatibility reasons.

in some instances the packing is being applied to structures where it is
essentially a noop. i.e. natural alignment gets you the same thing so it
is superfluous.

in some instances the packing is being applied to structures that are
private and it appears to be completely unnecessary e.g. some structure
that isn't nested into something else and sizeof() or offsetof() fields
don't matter in the context of their use.

in some instances it is completely necessary usually when type punning
buffers containing network framing etc...

unfortunately the standard doesn't offer me an out here as there is an
issue of placement of the pragma/attributes that do the packing.

for places it isn't needed it, whatever i just expand empty. for places
it is superfluous again because msvc has no stable abi (we're not
established yet) again i just expand empty. finally for the places where
it is needed i'll probably need to expand conditionally but i think the
instances are far fewer than current use.

> 
> The same risk applies to __rte_aligned(), but with lower probability.

so that's the long winded story of why they are both expanded empty for
now for msvc. but when the time comes i want to submit patch series that
focus on each specifically to generate robust discussion.

ty