>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_*.
Hi David,
I will send a patch to fix RTE_ALIGN_MUL_* + testcase to verify side-effect.
I tried to fix the RTE_ALIGN_* too but looks like since they are used to
statically
set array sizes it’s a bit complicated to fix them
ex.
../lib/eal/include/rte_common.h:311:2: error: braced-group within expression
allowed only inside a function
311 | ({
\
| ^
../lib/eal/include/rte_common.h:333:31: note: in expansion of macro
‘RTE_ALIGN_CEIL’
333 | #define RTE_ALIGN(val, align) RTE_ALIGN_CEIL(val, align)
| ^~~~~~~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:435:3: note: in expansion of macro ‘RTE_ALIGN’
435 | (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
| ^~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:452:27: note: in expansion of macro
‘RTE_FLOW_MASK_ARRAY_SIZE’
452 | uint64_t flow_types_mask[RTE_FLOW_MASK_ARRAY_SIZE];
I will send a separate patch to fix RTE_ALIGN* stuff and we can move forward
based on acceptance.
Regards,
Pavan.
>
>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
