Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[David Marchand]

On Wed, May 5, 2021 at 6:30 PM Tyler Retzlaff
<roret...@linux.microsoft.com> wrote:
>
> On Fri, Mar 12, 2021 at 09:07:22AM +0100, David Marchand wrote:
> > On Thu, Mar 11, 2021 at 10:08 PM Tyler Retzlaff
> > <roret...@linux.microsoft.com> wrote:
> > >
> > > Avoid expanding v and mul parameters multiple times in the macro. based
> > > on usage of the macro it seems like side effects were not intended.
> > >
> > > For example:
> > >   ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''
> >
> > That's the beauty of macros.
> > How about updating the unit tests so that this kind of issue is not
> > reintroduced?
>
> i'm afraid i don't have the schedule budget to do this. i get very
> little dpdk time.

I started to write a test earlier (see below) but I did not finish either.
We can wait until somebody has the time to investigate/finish the work.
I copied Pavan who authored RTE_ALIGN_MUL_*.

There are more macros affected than the one you fixed (see commented
lines with FIXME:), and there are probably more macros to check in
rte_common.h:

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 12bd1cad90..44770722a7 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -18,6 +18,13 @@
        {printf(x "() test failed!\n");\
        return -1;}

+static uintptr_t callcount;
+static uintptr_t
+inc_callcount(void)
+{
+       return callcount++;
+}
+
 /* this is really a sanity check */
 static int
 test_macros(int __rte_unused unused_parm)
@@ -28,8 +35,18 @@ test_macros(int __rte_unused unused_parm)
 #define FAIL_MACRO(x)\
        {printf(#x "() test failed!\n");\
        return -1;}
+#define TEST_SIDE_EFFECT_2(macro, type1, type2) do {                 \
+       callcount = 0;                                               \
+       (void)macro((type1)inc_callcount(), (type2)inc_callcount()); \
+       if (callcount != 2) {                                        \
+               printf(#macro" has side effects: callcount=%u\n",    \
+                       (unsigned int)callcount);                    \
+               ret = -1;                                            \
+       }                                                            \
+} while (0)

        uintptr_t unused = 0;
+       int ret = 0;

        RTE_SET_USED(unused);

@@ -47,7 +64,19 @@ test_macros(int __rte_unused unused_parm)
        if (strncmp(RTE_STR(test), "test", sizeof("test")))
                FAIL_MACRO(RTE_STR);

-       return 0;
+       TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
+       TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
+       TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t); */
+       TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int,
unsigned int); */
+       TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned int);
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int,
unsigned int); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned
int, unsigned int); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int,
unsigned int); */
+       return ret;
 }

 static int



-- 
David Marchand