DPDK patches and discussions
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: dev@dpdk.org
Subject: static_assert, sfc, and clang issues
Date: Tue, 16 Jan 2024 09:03:01 -0800	[thread overview]
Message-ID: <20240116090301.4c29d5b7@hermes.local> (raw)

Ran into a corner case issue, so sending to mailing list for wider discussion.

One improvement to DPDK code base planned is getting rid of variable length arrays.
VLA's can cause bugs and are not supported by the Windows compiler.
Gcc and Clang have a flag to warn on use of VLA's (-Wvla).

In DPDK one use of VLA's is in the RTE_BUILD_BUG_ON macro.
The best path is to replace the undefined array access with a better
static_assert() which is builtin to compilers.

Using static_assert() is relatively easy, there are a few places where
it does detect use of non-constant expressions in existing code but these
are fixable.

But there is one case where use static_assert() runs into a Clang bug
which is not fixed in distros: 

https://github.com/llvm/llvm-project/issues/55821

The code in question is in the sfc driver which for better or worse
has lots of assertions. The problem is that clang (except in unreleased trunk)
can not handle static_assert in a switch leg.

		switch (actions->type) {
		case RTE_FLOW_ACTION_TYPE_VOID:
			SFC_BUILD_SET_OVERFLOW(RTE_FLOW_ACTION_TYPE_VOID,
					       actions_set);
			break;

../drivers/net/sfc/sfc_flow.c:1635:4: error: expected expression
                        SFC_BUILD_SET_OVERFLOW(RTE_FLOW_ACTION_TYPE_VOID,
                        ^
../drivers/net/sfc/sfc_flow.h:36:2: note: expanded from macro 'SFC_BUILD_SET_OVERFLOW'
        RTE_BUILD_BUG_ON((_action) >= sizeof(_set) * CHAR_BIT)
        ^


There are some workarounds:
	0. Ignore it, works on Gcc, and Clang fix is pending.
	1. Remove many of these RTE_BUILD_BUG_ON's. They are really not that helpful.
	2. Add additional { } to these switch cases so they become basic blocks
	   which works around the bug.
        3. Move the assertions out of the switch

My preference is #2 but open to other options.

             reply	other threads:[~2024-01-16 17:03 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-16 17:03 Stephen Hemminger [this message]
2024-01-16 21:50 ` Tyler Retzlaff
2024-01-16 22:14   ` Morten Brørup
2024-01-16 22:18     ` Morten Brørup
2024-01-16 22:49     ` Stephen Hemminger
2024-01-17  8:07       ` Andrew Rybchenko
2024-01-17  9:37       ` Morten Brørup

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240116090301.4c29d5b7@hermes.local \
    --to=stephen@networkplumber.org \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).