Re: [PATCH v13 3/3] drivers/net: add diagnostics macros to make code portable

[Andre Muezerie <andremue@linux.microsoft.com>]

On Thu, Jan 16, 2025 at 10:08:07AM +0100, Morten Brørup wrote:
> > From: Andre Muezerie [mailto:andremue@linux.microsoft.com]
> > Sent: Thursday, 16 January 2025 02.55
> > 
> > It was a common pattern to have "GCC diagnostic ignored" pragmas
> > sprinkled over the code and only activate these pragmas for certain
> > compilers (gcc and clang). Clang supports GCC's pragma for
> > compatibility with existing source code, so #pragma GCC diagnostic
> > and #pragma clang diagnostic are synonyms for Clang
> > (https://clang.llvm.org/docs/UsersManual.html).
> > 
> > Now that effort is being made to make the code compatible with MSVC
> > these expressions would become more complex. It makes sense to hide
> > this complexity behind macros. This makes maintenance easier as these
> > macros are defined in a single place. As a plus the code becomes
> > more readable as well.
> 
> Here is some food for thought and discussion...
> 
> > @@ -2083,7 +2075,7 @@ dpaa2_dev_loopback_rx(void *queue,
> >  			if (unlikely((status & QBMAN_DQ_STAT_VALIDFRAME) ==
> > 0))
> >  				continue;
> >  		}
> > -		fd[num_rx] = (struct qbman_fd *)qbman_result_DQ_fd(dq_storage);
> > +		fd[num_rx] = RTE_PTR_DROP_QUALIFIERS(qbman_result_DQ_fd(dq_storage));
> 
> I do not think this makes the code more readable; quite the opposite.
> Before this, I could see which type the variable was being cast to.
> 
> How about a macro that resembles "traditional" type casting:
> 
> /**
>  * Workaround to discard qualifiers (such as const, volatile, restrict) from a pointer,
>  * without the compiler emitting a warning.
>  *
>  * @warning
>  * Although this macro can be abused for casting a pointer to point to a different type,
>  * alignment may be incorrect when casting to point to a larger type. E.g.:
>  *   struct s {
>  *       uint16_t a;
>  *       uint8_t  b;
>  *       uint8_t  c;
>  *       uint8_t  d;
>  *   } v;
>  *   uint16_t * p = RTE_CAST_PTR(uint16_t *, &v.c); // "p" is not 16 bit aligned!
>  */
> #define RTE_CAST_PTR(type, ptr) \
> 	((type)(uintptr_t)(ptr))
> 
> 
> Writing the above warning lead me down another path...
> Can we somehow use __typeof_unqual__?
> It is available in both GCC [1] and MSVC [2].
> 
> [1]: https://gcc.gnu.org/onlinedocs/gcc/Typeof.html
> [2]: https://learn.microsoft.com/en-us/cpp/c-language/typeof-unqual-c?view=msvc-170
> 
> 
> We are making a workaround, and should take care to not endorse overusing it.
> Especially for other purposes than intended.
> 
> Unfortunately, I think some of the type casts don't just remove qualifiers, but does exactly what my warning above describes: Casts a pointer to completely different type.
> If the new type is a larger type, the pointer's alignment becomes invalid, and if the compiler considers alignment a "qualifier", -Wcast-qual emits a warning about it.
> 
> 
> Backtracking a bit...
> If the macro is intended to remove qualifiers, and not to cast to a different type, RTE_PTR_DROP_QUALIFIERS(ptr) might be better than RTE_CAST_PTR(type, ptr).
> For brevity and to resemble the C23 keyword typeof_unqual, it could be named RTE_PTR_UNQUAL instead of RTE_PTR_DROP_QUALIFIERS.
> 

These are great suggestions, and __typeof_unqual__ seems to be exactly what we need to drop the qualifiers. I'll look more closely at the code and find out where a cast is actually being used for other purposes than removing the qualifier.