[0/3] Fix xmm_t to rte_xmm_t scalar conversion

  As David noticed in [1] there is an issue with C++ compilation of the
rte_vect.h header in RISC-V. Upon closer inspection, the problem appears on
all architectures due to the type conversion rules in C++.
More precisely a union type rte_xmm_t requires a conversion constructor
from xmm_t type.
The most obvious fix is to use a structure initializer for such copies
(since rte_xmm_t union contains xmm_t anyway). The generated assembly
at -O2 is exactly the same, so there's no real impact.

The bigger question is whether accessing bits of the architecture specific
xmm_t type in an array fashion is always correct? All current architectures
define rte_xmm_t in the same manner implying that.

Additionally change RISC-V CI settings to use crossbuild-essential-riscv64
package which provides tools that enable C++ checks.

[1] http://mails.dpdk.org/archives/dev/2022-June/243683.html

Stanislaw Kardach (3):
  eal/riscv: fix xmm_t casting for C++
  lpm: fix xmm_t casting for C++ in scalar version
  ci: use crossbuild-essential-riscv64 for compiling

 .github/workflows/build.yml      |  3 +--
 lib/eal/riscv/include/rte_vect.h |  4 ++--
 lib/lpm/rte_lpm_scalar.h         | 11 ++++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)
  

On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> As David noticed in [1] there is an issue with C++ compilation of the
> rte_vect.h header in RISC-V. Upon closer inspection, the problem appears on
> all architectures due to the type conversion rules in C++.
> More precisely a union type rte_xmm_t requires a conversion constructor
> from xmm_t type.
> The most obvious fix is to use a structure initializer for such copies
> (since rte_xmm_t union contains xmm_t anyway). The generated assembly
> at -O2 is exactly the same, so there's no real impact.
>
> The bigger question is whether accessing bits of the architecture specific
> xmm_t type in an array fashion is always correct? All current architectures
> define rte_xmm_t in the same manner implying that.

Copying other arch maintainers.

>
> Additionally change RISC-V CI settings to use crossbuild-essential-riscv64
> package which provides tools that enable C++ checks.
>
> [1] http://mails.dpdk.org/archives/dev/2022-June/243683.html
>
> Stanislaw Kardach (3):
>   eal/riscv: fix xmm_t casting for C++
>   lpm: fix xmm_t casting for C++ in scalar version
>   ci: use crossbuild-essential-riscv64 for compiling

In any case, this series looks good.
Series applied, thanks Stanislaw.
  

> On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
>>
>> As David noticed in [1] there is an issue with C++ compilation of the
>> rte_vect.h header in RISC-V. Upon closer inspection, the problem appears on
>> all architectures due to the type conversion rules in C++.
>> More precisely a union type rte_xmm_t requires a conversion constructor
>> from xmm_t type.
>> The most obvious fix is to use a structure initializer for such copies
>> (since rte_xmm_t union contains xmm_t anyway). The generated assembly
>> at -O2 is exactly the same, so there's no real impact.
>>
>> The bigger question is whether accessing bits of the architecture specific
>> xmm_t type in an array fashion is always correct? All current architectures
>> define rte_xmm_t in the same manner implying that.
> 
> Copying other arch maintainers.

My read of the Altivec vector layout for LE systems says the existing 
union operator rte_xmm_t is correct, though my C++ experience is 
limited.  How can I generate an error with C++ to expose this issue?

Dave
  

On Tue, Jun 21, 2022 at 12:54 AM David Christensen
<drc@linux.vnet.ibm.com> wrote:
>
> > On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
> >>
> >> As David noticed in [1] there is an issue with C++ compilation of the
> >> rte_vect.h header in RISC-V. Upon closer inspection, the problem appears on
> >> all architectures due to the type conversion rules in C++.
> >> More precisely a union type rte_xmm_t requires a conversion constructor
> >> from xmm_t type.
> >> The most obvious fix is to use a structure initializer for such copies
> >> (since rte_xmm_t union contains xmm_t anyway). The generated assembly
> >> at -O2 is exactly the same, so there's no real impact.
> >>
> >> The bigger question is whether accessing bits of the architecture specific
> >> xmm_t type in an array fashion is always correct? All current architectures
> >> define rte_xmm_t in the same manner implying that.
> >
> > Copying other arch maintainers.
>
> My read of the Altivec vector layout for LE systems says the existing
> union operator rte_xmm_t is correct, though my C++ experience is
> limited.  How can I generate an error with C++ to expose this issue?
>
> Dave
To replicate this issue:
1. Apply the patch below. In essence it forces the use of scalar lpm
and changes C++ compiler to g++ so that meson properly detects it.
Otherwise C++ checks won't be generated.
2. Configure build with: meson build-ppc64le --werror --cross-file
config/ppc/ppc64le-power8-linux-gcc-ubuntu -Dcheck_includes=true
3. Build with: ninja -C build-ppc64le buildtools/chkincs/chkincs-cpp

Note that the build target only gets generated if C++ compiler is
properly discovered by meson. To be honest I'm not sure why
powerpc64le-linux-gnu-cpp doesn't get properly picked up by meson.
Also of interesting note the similar substitution should be made for
arm64 but it seems like a separate thread.

Patch for issue replication:

diff --git a/config/arm/arm64_armv8_linux_gcc b/config/arm/arm64_armv8_linux_gcc
index 5391d35389..5c32f6b9ca 100644
--- a/config/arm/arm64_armv8_linux_gcc
+++ b/config/arm/arm64_armv8_linux_gcc
@@ -1,6 +1,6 @@
 [binaries]
 c = 'aarch64-linux-gnu-gcc'
-cpp = 'aarch64-linux-gnu-cpp'
+cpp = 'aarch64-linux-gnu-g++'
 ar = 'aarch64-linux-gnu-gcc-ar'
 strip = 'aarch64-linux-gnu-strip'
 pkgconfig = 'aarch64-linux-gnu-pkg-config'
diff --git a/config/ppc/ppc64le-power8-linux-gcc-ubuntu
b/config/ppc/ppc64le-power8-linux-gcc-ubuntu
index 803c612cbc..2e6b13a406 100644
--- a/config/ppc/ppc64le-power8-linux-gcc-ubuntu
+++ b/config/ppc/ppc64le-power8-linux-gcc-ubuntu
@@ -1,6 +1,6 @@
 [binaries]
 c = 'powerpc64le-linux-gnu-gcc'
-cpp = 'powerpc64le-linux-gnu-cpp'
+cpp = 'powerpc64le-linux-gnu-g++'
 ar = 'powerpc64le-linux-gnu-ar'
 strip = 'powerpc64le-linux-gnu-strip'

diff --git a/lib/lpm/rte_lpm.h b/lib/lpm/rte_lpm.h
index 4f38864fde..fd1362a027 100644
--- a/lib/lpm/rte_lpm.h
+++ b/lib/lpm/rte_lpm.h
@@ -397,19 +397,7 @@ static inline void
 rte_lpm_lookupx4(const struct rte_lpm *lpm, xmm_t ip, uint32_t hop[4],
        uint32_t defv);

-#if defined(RTE_ARCH_ARM)
-#ifdef RTE_HAS_SVE_ACLE
-#include "rte_lpm_sve.h"
-#else
-#include "rte_lpm_neon.h"
-#endif
-#elif defined(RTE_ARCH_PPC_64)
-#include "rte_lpm_altivec.h"
-#elif defined(RTE_ARCH_X86)
-#include "rte_lpm_sse.h"
-#else
 #include "rte_lpm_scalar.h"
-#endif

 #ifdef __cplusplus
 }
  

On Tue, Jun 21, 2022 at 11:30:32AM +0200, Stanisław Kardach wrote:
> On Tue, Jun 21, 2022 at 12:54 AM David Christensen
> <drc@linux.vnet.ibm.com> wrote:
> >
> > > On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
> > >>
> > >> As David noticed in [1] there is an issue with C++ compilation of the
> > >> rte_vect.h header in RISC-V. Upon closer inspection, the problem appears on
> > >> all architectures due to the type conversion rules in C++.
> > >> More precisely a union type rte_xmm_t requires a conversion constructor
> > >> from xmm_t type.
> > >> The most obvious fix is to use a structure initializer for such copies
> > >> (since rte_xmm_t union contains xmm_t anyway). The generated assembly
> > >> at -O2 is exactly the same, so there's no real impact.
> > >>
> > >> The bigger question is whether accessing bits of the architecture specific
> > >> xmm_t type in an array fashion is always correct? All current architectures
> > >> define rte_xmm_t in the same manner implying that.
> > >
> > > Copying other arch maintainers.
> >
> > My read of the Altivec vector layout for LE systems says the existing
> > union operator rte_xmm_t is correct, though my C++ experience is
> > limited.  How can I generate an error with C++ to expose this issue?
> >
> > Dave
> To replicate this issue:
> 1. Apply the patch below. In essence it forces the use of scalar lpm
> and changes C++ compiler to g++ so that meson properly detects it.
> Otherwise C++ checks won't be generated.
> 2. Configure build with: meson build-ppc64le --werror --cross-file
> config/ppc/ppc64le-power8-linux-gcc-ubuntu -Dcheck_includes=true
> 3. Build with: ninja -C build-ppc64le buildtools/chkincs/chkincs-cpp
> 
> Note that the build target only gets generated if C++ compiler is
> properly discovered by meson. To be honest I'm not sure why
> powerpc64le-linux-gnu-cpp doesn't get properly picked up by meson.

Generally the "cpp" binary is not the c-plus-plus one, but the C
preprocessor one. Perhaps the original files are incorrect here, and should
all refer to g++.

/Bruce
  

On Tue, Jun 21, 2022 at 11:39 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> <snip>
> Generally the "cpp" binary is not the c-plus-plus one, but the C
> preprocessor one. Perhaps the original files are incorrect here, and should
> all refer to g++.
>
> /Bruce
>
That does make sense. I'll submit a separate patchset fixing all
occurrences (of which there are many).
  

On Tue, Jun 21, 2022 at 11:42:55AM +0200, Stanisław Kardach wrote:
> On Tue, Jun 21, 2022 at 11:39 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > <snip>
> > Generally the "cpp" binary is not the c-plus-plus one, but the C
> > preprocessor one. Perhaps the original files are incorrect here, and should
> > all refer to g++.
> >
> > /Bruce
> >
> That does make sense. I'll submit a separate patchset fixing all
> occurrences (of which there are many).
>

As a more general note for future consideration, I notice that in meson
0.56 the cross-file support has been enhanced with the ability to use
constants and therefore separate out prefixes.[1]

When we get to the point where we feel we can mandate meson 0.56 upwards
for cross compilation, we should look to leverage this. It should even
allow other scripts such as test-meson-builds to auto-generate the constant
paths to the binaries on the fly, effectively allowing the use of
environment variables for these - something previously requested by Thomas.

/Bruce

[1] https://mesonbuild.com/Machine-files.html#constants
  

21/06/2022 11:49, Bruce Richardson:
> On Tue, Jun 21, 2022 at 11:42:55AM +0200, Stanisław Kardach wrote:
> > On Tue, Jun 21, 2022 at 11:39 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > > <snip>
> > > Generally the "cpp" binary is not the c-plus-plus one, but the C
> > > preprocessor one. Perhaps the original files are incorrect here, and should
> > > all refer to g++.
> > >
> > > /Bruce
> > >
> > That does make sense. I'll submit a separate patchset fixing all
> > occurrences (of which there are many).
> >
> 
> As a more general note for future consideration, I notice that in meson
> 0.56 the cross-file support has been enhanced with the ability to use
> constants and therefore separate out prefixes.[1]
> 
> When we get to the point where we feel we can mandate meson 0.56 upwards
> for cross compilation, we should look to leverage this. It should even
> allow other scripts such as test-meson-builds to auto-generate the constant
> paths to the binaries on the fly, effectively allowing the use of
> environment variables for these - something previously requested by Thomas.

That would be great.
Cross compilation prefix is such a basic thing, we should handle it properly.

> [1] https://mesonbuild.com/Machine-files.html#constants
  

On Tue, Jun 21, 2022 at 12:22 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 21/06/2022 11:49, Bruce Richardson:
> > On Tue, Jun 21, 2022 at 11:42:55AM +0200, Stanisław Kardach wrote:
> > > On Tue, Jun 21, 2022 at 11:39 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > <snip>
> > > > Generally the "cpp" binary is not the c-plus-plus one, but the C
> > > > preprocessor one. Perhaps the original files are incorrect here, and should
> > > > all refer to g++.
> > > >
> > > > /Bruce
> > > >
> > > That does make sense. I'll submit a separate patchset fixing all
> > > occurrences (of which there are many).
> > >
> >
> > As a more general note for future consideration, I notice that in meson
> > 0.56 the cross-file support has been enhanced with the ability to use
> > constants and therefore separate out prefixes.[1]
> >
> > When we get to the point where we feel we can mandate meson 0.56 upwards
> > for cross compilation, we should look to leverage this. It should even
> > allow other scripts such as test-meson-builds to auto-generate the constant
> > paths to the binaries on the fly, effectively allowing the use of
> > environment variables for these - something previously requested by Thomas.
>
> That would be great.
> Cross compilation prefix is such a basic thing, we should handle it properly.
Please correct me if I'm wrong but it seems that meson's approach to
cross-compiling is to package all settings into cross-files. Probably
under assumption that a repeatable compilation is more important than
flexibility and that there are compiler-specific knobs that need/can
to be tuned. Therefore reading CROSS_COMPILE/prefix from environment
is intentionally made hard.

So should the direction be environment or rather separating
cross-files into arch-part and toolchain-parts and letting user create
his own toolchain part while maintaining a matrix of supported
combinations for CI? I'm not advocating either, just want to wrap my
head around it.

>
> > [1] https://mesonbuild.com/Machine-files.html#constants
>
>
>
  

21/06/2022 13:05, Stanisław Kardach:
> On Tue, Jun 21, 2022 at 12:22 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 21/06/2022 11:49, Bruce Richardson:
> > > On Tue, Jun 21, 2022 at 11:42:55AM +0200, Stanisław Kardach wrote:
> > > > On Tue, Jun 21, 2022 at 11:39 AM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > > <snip>
> > > > > Generally the "cpp" binary is not the c-plus-plus one, but the C
> > > > > preprocessor one. Perhaps the original files are incorrect here, and should
> > > > > all refer to g++.
> > > > >
> > > > > /Bruce
> > > > >
> > > > That does make sense. I'll submit a separate patchset fixing all
> > > > occurrences (of which there are many).
> > > >
> > >
> > > As a more general note for future consideration, I notice that in meson
> > > 0.56 the cross-file support has been enhanced with the ability to use
> > > constants and therefore separate out prefixes.[1]
> > >
> > > When we get to the point where we feel we can mandate meson 0.56 upwards
> > > for cross compilation, we should look to leverage this. It should even
> > > allow other scripts such as test-meson-builds to auto-generate the constant
> > > paths to the binaries on the fly, effectively allowing the use of
> > > environment variables for these - something previously requested by Thomas.
> >
> > That would be great.
> > Cross compilation prefix is such a basic thing, we should handle it properly.
> Please correct me if I'm wrong but it seems that meson's approach to
> cross-compiling is to package all settings into cross-files. Probably
> under assumption that a repeatable compilation is more important than
> flexibility and that there are compiler-specific knobs that need/can
> to be tuned. Therefore reading CROSS_COMPILE/prefix from environment
> is intentionally made hard.

If it is made intentionally hard, it is just a wrong design.
A toolchain prefix is just a name.
We can have 2 toolchains compiled with the same name and different behaviours.
And we can have 2 similar toolchains with a different name.

> So should the direction be environment or rather separating
> cross-files into arch-part and toolchain-parts and letting user create
> his own toolchain part while maintaining a matrix of supported
> combinations for CI? I'm not advocating either, just want to wrap my
> head around it.

We should be able to use a toolchain compiled anywhere
without modifying the cross files, just because a "-gnu-" is missing
or any other irrelevant detail.
  

On Tue, Jun 21, 2022 at 1:53 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> <snip>
> If it is made intentionally hard, it is just a wrong design.
> A toolchain prefix is just a name.
> We can have 2 toolchains compiled with the same name and different behaviours.
> And we can have 2 similar toolchains with a different name.
I don't think meson will allow it anytime soon (see [1]). The
reasoning being that it's easy to screw up the environment and not
notice it where as files are persistent.
>
> > So should the direction be environment or rather separating
> > cross-files into arch-part and toolchain-parts and letting user create
> > his own toolchain part while maintaining a matrix of supported
> > combinations for CI? I'm not advocating either, just want to wrap my
> > head around it.
>
> We should be able to use a toolchain compiled anywhere
> without modifying the cross files, just because a "-gnu-" is missing
> or any other irrelevant detail.
I've checked that if I remove the binaries from a cross-file, then
specifying CC/CXX/AR/STRIP environment variables is picked up by
meson:
  AR=riscv64-unknown-linux-gnu-ar \
    STRIP=riscv64-unknown-linux-gnu-strip \
    CC=riscv64-unknown-linux-gnu-gcc \
    CXX=riscv64-unknown-linux-gnu-g++ \
    meson build-rv-test --cross-file=config/riscv/riscv64_linux_gcc
But then there are no default values.

A suggested frankenstein-like solution in [1] is to use a script that
generates a cross-file with [constants] section and launches meson
with it.

[1] https://github.com/mesonbuild/meson/issues/9#issuecomment-381410972
  

On Tue, Jun 21, 2022 at 02:37:51PM +0200, Stanisław Kardach wrote:
> On Tue, Jun 21, 2022 at 1:53 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > <snip>
> > If it is made intentionally hard, it is just a wrong design.
> > A toolchain prefix is just a name.
> > We can have 2 toolchains compiled with the same name and different behaviours.
> > And we can have 2 similar toolchains with a different name.
> I don't think meson will allow it anytime soon (see [1]). The
> reasoning being that it's easy to screw up the environment and not
> notice it where as files are persistent.
> >
> > > So should the direction be environment or rather separating
> > > cross-files into arch-part and toolchain-parts and letting user create
> > > his own toolchain part while maintaining a matrix of supported
> > > combinations for CI? I'm not advocating either, just want to wrap my
> > > head around it.
> >
> > We should be able to use a toolchain compiled anywhere
> > without modifying the cross files, just because a "-gnu-" is missing
> > or any other irrelevant detail.
> I've checked that if I remove the binaries from a cross-file, then
> specifying CC/CXX/AR/STRIP environment variables is picked up by
> meson:
>   AR=riscv64-unknown-linux-gnu-ar \
>     STRIP=riscv64-unknown-linux-gnu-strip \
>     CC=riscv64-unknown-linux-gnu-gcc \
>     CXX=riscv64-unknown-linux-gnu-g++ \
>     meson build-rv-test --cross-file=config/riscv/riscv64_linux_gcc
> But then there are no default values.
> 
> A suggested frankenstein-like solution in [1] is to use a script that
> generates a cross-file with [constants] section and launches meson
> with it.
> 
> [1] https://github.com/mesonbuild/meson/issues/9#issuecomment-381410972

This is all incidental to the original fix, which is to replace cpp with
g++ in the existing cross files. Any update to how cross files are used by
our DPDK scripts, is a different discussion for a different day!
[Sorry to have brought it up here, it just seemed somewhat relevant at the
time!]
  

On Tue, Jun 21, 2022 at 11:42:55AM +0200, Stanisław Kardach wrote:
> On Tue, Jun 21, 2022 at 11:39 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > <snip>
> > Generally the "cpp" binary is not the c-plus-plus one, but the C
> > preprocessor one. Perhaps the original files are incorrect here, and should
> > all refer to g++.
> >
> > /Bruce

agreed cpp is the preprocessor and one of 2 conventions for file
extension of c++ files (the other being .cc)

conventionally you're looking for CXX i.e. as in env CXX=g++

> >
> That does make sense. I'll submit a separate patchset fixing all
> occurrences (of which there are many).
> 
> -- 
> Best Regards,
> Stanisław Kardach