On Wed, Jan 08, 2025 at 09:20:27AM +0000, Bruce Richardson wrote:
> On Tue, Jan 07, 2025 at 06:46:48PM -0800, Andre Muezerie wrote:
> > On Mon, Jan 06, 2025 at 11:00:15AM +0000, Bruce Richardson wrote:
> > > On Fri, Jan 03, 2025 at 01:26:34PM -0800, Andre Muezerie wrote:
> > > > On Fri, Jan 03, 2025 at 11:24:02AM -0800, Stephen Hemminger wrote:
> > > > > On Fri, 3 Jan 2025 07:36:48 -0800
> > > > > Andre Muezerie <[email protected]> wrote:
> > > > >
> > > > > > From: Andre Muezerie <[email protected]>
> > > > > > To: [email protected]
> > > > > > Cc: [email protected], [email protected]
> > > > > > Subject: [PATCH v11 0/3] add diagnostics macros to make code
> > > > > > portable
> > > > > > Date: Fri, 3 Jan 2025 07:36:48 -0800
> > > > > > X-Mailer: git-send-email 1.8.3.1
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Since 90% of these cases are about removing const from a pointer,
> > > > > maybe it would be better to have a macro that did that?
> > > > >
> > > > > Would not work for base driver code which is pretending to be
> > > > > platform independent.
> > > >
> > > > Most of the warnings I've seen were about dropping the volatile
> > > > qualifier, like the one below:
> > > >
> > > > ../drivers/net/i40e/i40e_rxtx_vec_sse.c:42:32: warning: cast from
> > > > 'volatile struct i40e_32byte_rx_desc::(unnamed at
> > > > ../drivers/net/i40e/base/i40e_type.h:803:2) *' to
> > > > '__attribute__((__vector_size__(2 * sizeof(long long)))) long long *'
> > > > drops volatile qualifier [-Wcast-qual]
> > > > 42 | _mm_store_si128((__m128i
> > > > *)&rxdp[i].read,
> > > > | ^
> > > >
> > > > To make sure I understood your suggestion correctly, you're proposing
> > > > to replace this
> > > >
> > > > __rte_diagnostic_push
> > > > __rte_diagnostic_ignored_wcast_qual
> > > > _mm_store_si128((__m128i *)&rxdp[i].read, dma_addr0);
> > > > __rte_diagnostic_pop
> > > >
> > > >
> > > > with something like this?
> > > >
> > > > _mm_store_si128(RTE_IGNORE_CAST_QUAL((__m128i *)&rxdp[i].read),
> > > > dma_addr0);
> > > >
> > > > This could be done, and I think it does look better, despite the slight
> > > > line length increase.
> > >
> > > +1 for this option. One macro can be used to drop all qualifiers, both
> > > const and volatile, right?
> >
> > Yes, a single macro can drop all qualifiers. I did realize though that the
> > macro must involve the entire expression - it cannot be used just around
> > one parameter, unfortunately.
> >
> For many use cases, those involving pointers, the qualifiers can be cast
> away by passing through a uintptr_t. Just tested this with gcc and clang:
>
> volatile int x = 5;
> int *y = (int *)(uintptr_t)&x;
> printf("*y = %d\n", *y);
>
> works without warnings or errors. Does this similarly work with MSVC? If
> so, we can do a macro specifically for pointers types, which should cover
> 99% of what we need.
>
> /Bruce
Yes, that also works with MSVC. So for the macro you mentioned, is this what
you had in mind?
old code:
_mm_store_si128((__m128i *)&rxdp[i].read, dma_addr0);
new code:
#define RTE_IGNORE_CAST_QUAL(X) \
(uintptr_t)(X)
_mm_store_si128((__m128i *)RTE_IGNORE_CAST_QUAL(&rxdp[i].read), dma_addr0);
