On Tue, Apr 04, 2023 at 01:07:24PM -0700, Tyler Retzlaff wrote:
> 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 <roret...@linux.microsoft.com>
> ---
> lib/eal/include/rte_branch_prediction.h | 8 ++++++++
> lib/eal/include/rte_common.h | 33
> +++++++++++++++++++++++++++++++++
> lib/eal/include/rte_compat.h | 20 ++++++++++++++++++++
> 3 files changed, 61 insertions(+)
>
> diff --git a/lib/eal/include/rte_branch_prediction.h
> b/lib/eal/include/rte_branch_prediction.h
> index 0256a9d..3589c97 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) == 1)
> +#endif
I think this should just be "#define likely(x) (x)", since the likely is
just a hint as to which way we expect the branch to go. It does not change
the logic in the expression.
> #endif /* likely */
>
> /**
> @@ -39,7 +43,11 @@
> *
> */
> #ifndef unlikely
> +#ifndef RTE_TOOLCHAIN_MSVC
> #define unlikely(x) __builtin_expect(!!(x), 0)
> +#else
> +#define unlikely(x) (!!(x) == 0)
> +#endif
This expansion is wrong, because it changes the logic of the expression,
rather than being a hint. As above with likely, I think this should just
expand as "(x)", making the unlikely ignored.
> #endif /* unlikely */
>
> #ifdef __cplusplus
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 2f464e3..a724e22 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
This file has a lot of ifdefs in it now for msvc. Couple of suggestions:
1. can we group these defines together so we can hit multiple entries with a
single msvc block?
2. alternatively, for those we want to just permanently null, we could
split the responsibility for them between the currnet headers and possibly
the build system. Specifically, rather than doing macros based on MSVC,
change each macro to the simpler:
#ifndef MACRO
#define MACRO macro_definition
#endif
Then in the meson.build processing, we can have some separte MSVC
processing to put null defines for these into rte_build_config.h, or put
in null defines for them in some MSVC-specific header.
Just some thoughts.
/Bruce
