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 07:56:52PM -0800, Andre Muezerie wrote:
> 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.

I took a closer look at the code and this is what I found:

* Only 2 places where qualifiers were being dropped were not casting to a different type. I used RTE_PTR_UNQUAL in those as suggested, for clarity.

* I experimented with C23 typeof_unqual. It indeed works on gcc, clang and MSVC, but there are some details:
    a) With gcc the project needs to be compiled with -std=c2x. Many other warnings show up, unrelated to the scope of this patchset. Some look suspicious and should be looked at. An error also showed up, for which I sent out a small patch.
    b) When using typeof_unqual and passing "-Wcast-qual" to the compiler, a warning about the qualifier being dropped is emitted. The project currently uses "-Wcast-qual"
Due to (a) I decided to not use typeof_unqual for now, but it would be trivial to change the macro in the future to do so.

* All other places where I was using RTE_PTR_DROP_QUALIFIERS I'm using RTE_CAST_PTR now. I also think that the code became more readable by doing so.