> From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > Sent: Saturday, 15 April 2023 22.52 > > On Sat, Apr 15, 2023 at 09:16:21AM +0200, Morten Brørup wrote: > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > Sent: Friday, 14 April 2023 19.02 > > > > > > On Fri, Apr 14, 2023 at 08:45:17AM +0200, Morten Brørup wrote: > > > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > > > Sent: Thursday, 13 April 2023 23.26 > > > > > > > > > > For now expand a lot of common rte macros empty. The catch here > is > > > we > > > > > need to test that most of the macros do what they should but at > the > > > same > > > > > time they are blocking work needed to bootstrap of the unit > tests. > > > > > > > > > > Later we will return and provide (where possible) expansions > that > > > work > > > > > correctly for msvc and where not possible provide some alternate > > > macros > > > > > to achieve the same outcome. > > > > > > > > > > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com> > > > > [...] > > > > > > > /** > > > > > * Force alignment > > > > > */ > > > > > +#ifndef RTE_TOOLCHAIN_MSVC > > > > > #define __rte_aligned(a) __attribute__((__aligned__(a))) > > > > > +#else > > > > > +#define __rte_aligned(a) > > > > > +#endif > > > > > > > > It should be reviewed that __rte_aligned() is only used for > > > optimization purposes, and is not required for DPDK to function > > > properly. > > > > > > so to expand on what i have in mind (and explain why i leave it > expanded > > > empty for now) > > > > > > while msvc has a __declspec for align there is a mismatch between > > > where gcc and msvc want it placed to control alignment of objects. > > > > > > msvc support won't be functional in 23.07 because of atomics. so > once > > > we reach the 23.11 cycle (where we can merge c11 changes) it means > we > > > can also use standard _Alignas which can accomplish the same thing > > > but portably. > > > > That (C11 standard _Alignas) should be the roadmap for solving the > alignment requirements. > > > > This should be a general principle for DPDK... if the C standard > offers something, don't reinvent our own. And as a consequence of the > upgrade to C11, we should deprecate all our own now-obsolete substitutes > for these. > > > > > > > > full disclosure the catch is i still have to properly locate the > <thing> > > > that does the alignment and some small questions about the expansion > and > > > use of the existing macro. > > > > > > on the subject of DPDK requiring proper alignment, you're right it > > > is generally for performance but also for pre-c11 atomics. > > > > > > one question i have been asking myself is would the community see > value > > > in more compile time assertions / testing of the size and alignment > of > > > structures and offset of structure fields? we have a few key > > > RTE_BUILD_BUG_ON() assertions but i've discovered they don't offer > > > comprehensive protection. > > > > Absolutely. Catching bugs at build time is much better than any > alternative! > > that's handy feedback. i am now encouraged to include more compile time > checks in advance of or along with changes related to structure abi.
Sounds good. Disclaimer: "Absolutely" was my personal response. But I seriously doubt that anyone in the DPDK community would object to more build time checks. Stability and code quality carries a lot of weight in DPDK community discussions. With that said, please expect that maintainers might want you to split your patches, so the additional checks are separated from the MSVC changes. > follow on question, once we do get to use c11 would something like > _Static_assert be preferable over RTE_BUILD_BUG_ON? structures sensitive > to layout could be co-located with the asserts right at the point of > definition. or is there something extra RTE_BUILD_BUG_ON gives us? People may have different opinions on RTE_BUILD_BUG_ON vs. _Static_assert or static_assert. Personally, I prefer static_assert/_Static_assert. It also has the advantage that it can be used in the global scope, directly following the structure definitions (like you mention), whereas RTE_BUILD_BUG_ON must be inside a code block (which can probably be worked around by making a dummy static inline function only containing the RTE_BUILD_BUG_ON). And in the spirit of my proposal of not using home-grown macros as alternatives to what the C standard provides, I think we should deprecate and get rid of RTE_BUILD_BUG_ON in favor of static_assert/_Static_assert introduced by the C11 standard. (My personal opinion, no such principle decision has been made!) If we want to keep RTE_BUILD_BUG_ON for some reason, we could change its implementation to use static_assert/_Static_assert instead of creating an invalid pointer to make the compilation fail. > > > > > > > > /** > > > > > * Force a structure to be packed > > > > > */ > > > > > +#ifndef RTE_TOOLCHAIN_MSVC > > > > > #define __rte_packed __attribute__((__packed__)) > > > > > +#else > > > > > +#define __rte_packed > > > > > +#endif > > > > > > > > Similar comment as for __rte_aligned(); however, I consider it > more > > > likely that structure packing is a functional requirement, and not > just > > > used for optimization. Based on my experience, it may be used for > > > packing network structures; perhaps not in DPDK itself but maybe in > DPDK > > > applications. > > > > > > so interestingly i've discovered this is kind of a mess and as you > note > > > some places we can't just "fix" it for abi compatibility reasons. > > > > > > in some instances the packing is being applied to structures where > it is > > > essentially a noop. i.e. natural alignment gets you the same thing > so it > > > is superfluous. > > > > > > in some instances the packing is being applied to structures that > are > > > private and it appears to be completely unnecessary e.g. some > structure > > > that isn't nested into something else and sizeof() or offsetof() > fields > > > don't matter in the context of their use. > > > > > > in some instances it is completely necessary usually when type > punning > > > buffers containing network framing etc... > > > > > > unfortunately the standard doesn't offer me an out here as there is > an > > > issue of placement of the pragma/attributes that do the packing. > > > > > > for places it isn't needed it, whatever i just expand empty. for > places > > > it is superfluous again because msvc has no stable abi (we're not > > > established yet) again i just expand empty. finally for the places > where > > > it is needed i'll probably need to expand conditionally but i think > the > > > instances are far fewer than current use. > > > > Optimally, we will have a common macro (or other solution) to support > both GCC/CLANG and MSVC to replace or supplement __rte_packed. However, > the cost of this may be an API break if we replace __rte_packed. > > > > > > > > > > > > > The same risk applies to __rte_aligned(), but with lower > probability. > > > > > > so that's the long winded story of why they are both expanded empty > for > > > now for msvc. but when the time comes i want to submit patch series > that > > > focus on each specifically to generate robust discussion. > > > > Sounds like the right path to take. > > > > Now, I'm thinking ahead here... > > > > We should be prepared to accept a major ABI/API break at one point in > time, to replace our home-grown macros with C11 standard solutions and > to fully support MSVC. This is not happening anytime soon, but the > Techboard should acknowledge that this is going to happen (with an > unspecified release), so it can be formally announced. The sooner it is > announced, the more time developers will have to prepare for it. > > so, just to avoid any confusion i want to make it clear that i am not > planning to submit changes that would change abi as a part of supporting > msvc (aside from changing to standard atomics which we agreed on). Thank you for clarifying. > > in general there are some cleanups we could make in the area of code > maintainability and portability and we may want to discuss the > advantages or disadvantages of making those changes. but i think those > changes are a topic unrelated to windows or msvc specifically. This was the point I was trying to make, when I proposed accepting a major ABI/API break. Sorry about my unclear wording. If we collect a wish list of breaking changes, I would personally prefer a "big bang" major ABI/API break, rather than a series of incremental API/ABI breaks over multiple DPDK release. In this regard, we could mix both changes driven by the migration to pure C11 (e.g. getting rid of now-obsolete macros, such as RTE_BUILD_BUG_ON, and compiler intrinsics, such as __rte_aligned) and MSVC portability changes (e.g. an improved macro to support structure packing). > > > > > All the details do not need to be known at the time of the > announcement; they can be added along the way, based on the discussions > from your future patches. > > > > > > > > > ty
