[PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Stanislaw Kardach]

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(-)

-- 
2.30.2

[PATCH 1/3] eal/riscv: fix xmm_t casting for C++

[Stanislaw Kardach]

rte_xmm_t is a union type which wraps around xmm_t and maps its contents
to scalar structures. Since C++ has stricter type conversion rules than
C, the rte_xmm_t::x has to be used instead of C-casting.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
---
 lib/eal/riscv/include/rte_vect.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/riscv/include/rte_vect.h b/lib/eal/riscv/include/rte_vect.h
index 4600521c20..2f97f437a2 100644
--- a/lib/eal/riscv/include/rte_vect.h
+++ b/lib/eal/riscv/include/rte_vect.h
@@ -41,8 +41,8 @@ vect_load_128(void *p)
 static inline xmm_t
 vect_and(xmm_t data, xmm_t mask)
 {
-	rte_xmm_t ret = (rte_xmm_t)data;
-	rte_xmm_t m = (rte_xmm_t)mask;
+	rte_xmm_t ret = {.x = data };
+	rte_xmm_t m = {.x = mask };
 	ret.u64[0] &= m.u64[0];
 	ret.u64[1] &= m.u64[1];
 	return ret.x;
-- 
2.30.2

Re: [PATCH 1/3] eal/riscv: fix xmm_t casting for C++

[David Marchand]

On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> rte_xmm_t is a union type which wraps around xmm_t and maps its contents
> to scalar structures. Since C++ has stricter type conversion rules than
> C, the rte_xmm_t::x has to be used instead of C-casting.
>

Fixes: f22e705ebf12 ("eal/riscv: support RISC-V architecture")

> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

[PATCH 2/3] lpm: fix xmm_t casting for C++ in scalar version

[Stanislaw Kardach]

rte_xmm_t is a union type which wraps around xmm_t and maps its contents
to scalar structures. Since C++ has stricter type conversion rules than
C, the rte_xmm_t::x has to be used instead of C-casting.

The generated assembly is identical to the code without the fix (checked
both on x86 and RISC-V).

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
---
 lib/lpm/rte_lpm_scalar.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/lpm/rte_lpm_scalar.h b/lib/lpm/rte_lpm_scalar.h
index f0d9f37894..161b40ff80 100644
--- a/lib/lpm/rte_lpm_scalar.h
+++ b/lib/lpm/rte_lpm_scalar.h
@@ -15,18 +15,19 @@ extern "C" {
 
 static inline void
 rte_lpm_lookupx4(const struct rte_lpm *lpm, xmm_t ip, uint32_t hop[4],
-		uint32_t defv)
+		 uint32_t defv)
 {
+	rte_xmm_t xip = { .x = ip };
 	uint32_t nh;
 	int ret;
 
-	ret = rte_lpm_lookup(lpm, ((rte_xmm_t)ip).u32[0], &nh);
+	ret = rte_lpm_lookup(lpm, xip.u32[0], &nh);
 	hop[0] = (ret == 0) ? nh : defv;
-	ret = rte_lpm_lookup(lpm, ((rte_xmm_t)ip).u32[1], &nh);
+	ret = rte_lpm_lookup(lpm, xip.u32[1], &nh);
 	hop[1] = (ret == 0) ? nh : defv;
-	ret = rte_lpm_lookup(lpm, ((rte_xmm_t)ip).u32[2], &nh);
+	ret = rte_lpm_lookup(lpm, xip.u32[2], &nh);
 	hop[2] = (ret == 0) ? nh : defv;
-	ret = rte_lpm_lookup(lpm, ((rte_xmm_t)ip).u32[3], &nh);
+	ret = rte_lpm_lookup(lpm, xip.u32[3], &nh);
 	hop[3] = (ret == 0) ? nh : defv;
 }
 
-- 
2.30.2

Re: [PATCH 2/3] lpm: fix xmm_t casting for C++ in scalar version

[David Marchand]

On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> rte_xmm_t is a union type which wraps around xmm_t and maps its contents
> to scalar structures. Since C++ has stricter type conversion rules than
> C, the rte_xmm_t::x has to be used instead of C-casting.
>
> The generated assembly is identical to the code without the fix (checked
> both on x86 and RISC-V).

Fixes: 406937f89ffd ("lpm: add scalar version of lookupx4")

>
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>


> ---
>  lib/lpm/rte_lpm_scalar.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/lib/lpm/rte_lpm_scalar.h b/lib/lpm/rte_lpm_scalar.h
> index f0d9f37894..161b40ff80 100644
> --- a/lib/lpm/rte_lpm_scalar.h
> +++ b/lib/lpm/rte_lpm_scalar.h
> @@ -15,18 +15,19 @@ extern "C" {
>
>  static inline void
>  rte_lpm_lookupx4(const struct rte_lpm *lpm, xmm_t ip, uint32_t hop[4],
> -               uint32_t defv)
> +                uint32_t defv)

Nit: unrelated whitespace change.
Besides, indent is supposed to be one tab.
This can be dropped when applying.


-- 
David Marchand

[PATCH 3/3] ci: use crossbuild-essential-riscv64 for compiling

[Stanislaw Kardach]

The current packages installed for RISC-V build check do not contain a
C++ compiler, which hid an issue with C++ type conversion in the
rte_vect.h header on RISC-V or in the scalar implementation of the LPM
x4 lookup. Now that this issue is fixed, use the full toolchain install
to enable the C++ test.

Besides, the user's guide for RISC-V cross-compilation recommends the
use of crossbuild-essential-riscv64.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
---
 .github/workflows/build.yml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7c8528cb04..c0d2829d0e 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -139,8 +139,7 @@ jobs:
         pkg-config-powerpc-linux-gnu
     - name: Install riscv64 cross compiling packages
       if: env.RISCV64 == 'true'
-      run: sudo apt install -y gcc-riscv64-linux-gnu libc6-dev-riscv64-cross
-        pkg-config-riscv64-linux-gnu
+      run: sudo apt install -y crossbuild-essential-riscv64
     - name: Install test tools packages
       if: env.AARCH64 != 'true' || env.PPC64LE != 'true' || env.RISCV64 != 'true' || env.RUN_TESTS == 'true'
       run: sudo apt install -y gdb
-- 
2.30.2

Re: [PATCH 3/3] ci: use crossbuild-essential-riscv64 for compiling

[David Marchand]

On Thu, Jun 9, 2022 at 2:17 PM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> The current packages installed for RISC-V build check do not contain a
> C++ compiler, which hid an issue with C++ type conversion in the
> rte_vect.h header on RISC-V or in the scalar implementation of the LPM
> x4 lookup. Now that this issue is fixed, use the full toolchain install
> to enable the C++ test.
>
> Besides, the user's guide for RISC-V cross-compilation recommends the
> use of crossbuild-essential-riscv64.
>
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH 3/3] ci: use crossbuild-essential-riscv64 for compiling

[Aaron Conole]

Stanislaw Kardach <kda@semihalf.com> writes:

> The current packages installed for RISC-V build check do not contain a
> C++ compiler, which hid an issue with C++ type conversion in the
> rte_vect.h header on RISC-V or in the scalar implementation of the LPM
> x4 lookup. Now that this issue is fixed, use the full toolchain install
> to enable the C++ test.
>
> Besides, the user's guide for RISC-V cross-compilation recommends the
> use of crossbuild-essential-riscv64.
>
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[David Marchand]

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.


-- 
David Marchand

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[David Christensen]

> 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

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Stanisław Kardach]

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
 }


-- 
Best Regards,
Stanisław Kardach

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Bruce Richardson]

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

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Stanisław Kardach]

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).

-- 
Best Regards,
Stanisław Kardach

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[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.

/Bruce

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

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Thomas Monjalon]

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 

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[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.

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
>
>
>

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Thomas Monjalon]

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.

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Stanisław Kardach]

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

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Bruce Richardson]

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!]

Re: [PATCH 0/3] Fix xmm_t to rte_xmm_t scalar conversion

[Tyler Retzlaff]

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