All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yodel Eldar <yodel.eldar@yodel.dev>
To: qemu-devel@nongnu.org
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Daniel P. Berrangé" <berrange@redhat.com>
Subject: Re: [RFC PATCH v2] configure: Use clang for sanitizer builds or disable Werror
Date: Tue, 3 Mar 2026 19:08:25 -0600	[thread overview]
Message-ID: <f999293f-c424-497a-8bca-2634bcd68108@yodel.dev> (raw)
In-Reply-To: <20260303012054.484837-1-yodel.eldar@yodel.dev>

+Daniel (thanks for your comments)

On 02/03/2026 19:20, Yodel Eldar wrote:
> From: Yodel Eldar <yodel.eldar@yodel.dev>
> 
> Builds with --enable-{asan,tsan,safe-stack} fail under GCC, so use
> clang if available, otherwise disable the treatment of warnings as
> errors.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3006
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Yodel Eldar <yodel.eldar@yodel.dev>
> ---
> Hi,
> 
> The previous version only disabled Werror whenever `--skip-meson` wasn't
> used and the build occurred in a git repo, but this change should
> probably apply to all types of builds. So, let's use meson_option_add
> to globally disable Werror instead; IIUC (and according to my testing),
> this will override the value in config-meson.cross.new.
> 
> I'm still not sure if we should be disabling Werror for ubsan, even
> though it's not currently breaking builds with GCC; please let me know
> what you think.
> 
> Special thanks to Peter for looking into the cause of the reports around
> this, for sharing the findings, and suggesting approaches to resolve it.
> I couldn't pick one over the other, so I went with using clang when
> available with Werror disable as a fallback; please let me know if you
> think this is an XOR kind of policy decision.
> 
> Link to RFCv1:
> https://lore.kernel.org/qemu-devel/20260302210039.261325-1-yodel.eldar@yodel.dev/
> 
> Link to mentioned discussion:
> https://lore.kernel.org/qemu-devel/CAFEAcA88hc4UsgpuPXBWpbeN0tW26159kPn7jx2J9erBA5DLBw@mail.gmail.com/
> 
> v2:
> - Fix misnomer in commit message
> - Simplify condition by using the same variable for all sanitizers
> - Use meson_option_add to disable Werror
> 
> Thanks,
> Yodel
> ---
>   configure | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/configure b/configure
> index 5e114acea2..e457e8a17d 100755
> --- a/configure
> +++ b/configure
> @@ -762,6 +762,12 @@ for opt do
>     ;;
>     --wasm64-32bit-address-limit)
>     ;;
> +  --enable-asan) use_sanitizer="yes"
> +  ;;
> +  --enable-tsan) use_sanitizer="yes"
> +  ;;
> +  --enable-safe-stack) use_sanitizer="yes"
> +  ;;
>     # everything else has the same name in configure and meson
>     --*) meson_option_parse "$opt" "$optarg"
>     ;;
> @@ -771,6 +777,18 @@ for opt do
>     esac
>   done
>   
> +if test "$use_sanitizer" = "yes"; then
> +    if has clang; then
> +        echo "Sanitizer requested: setting compiler suite to clang"
> +        cc=clang
> +        cxx=clang++
> +        host_cc=clang
> +    else
> +        echo "Sanitizer requested: disabling Werror for non-clang compilers"
> +        meson_option_add -Dwerror=false
> +    fi
> +fi
> +
>   if ! test -e "$source_path/.git"
>   then
>       git_submodules_action="validate"

We could treat the rtl8139 as a one-off by carving out the
offending code with a GCC pragma or finagling GCC into cooperation
by substituting eth_payload_data with the expression assigned to it
(thank you, Thomas and Daniel, respectively); indeed, AFAICT the
rtl8139 is currently the only code that triggers the GCC bug
(and is overdue for a refactor/cleanup), so it's tempting to go with
either of these helpful suggestions; *however*, until the GCC team
fixes their buggy pairing between sanitizer and -Werror
(a pairing that they themselves disavow [1]), IMHO there's a
significant risk of recurrence if we went with either option, and
it's not clear to me whether reviewers will be able easily spot the
next one before it's too late (post-acceptance).

That said, I fully share Daniel's concerns about "overriding the
user's choice of compiler"; so, what if we instead moved the
sanitizer check into the existing "Preferred compiler" section
in configure, such that we only set cc=clang when the user hasn't
explicitly specified CC/CXX (diff below)? Note: there's already
precedent for this with the Obj-C compiler [2].

I'm definitely onboard with Daniel's warning message in meson whenever
the user: 1) explicitly opts for gcc/g++, 2) enables sanitizers, and 3)
-Werror is enabled. At the risk of overengineering, though, what if we
made it an error message instead, and gave users an escape hatch like
`--force-werror-sanitizers` (name TBD)? I can see that being too much,
but it may prevent some grief if they missed the warning, and the build
breaks several minutes later. WDYT? It's not included in the diff below,
but I'll gladly add it to v3 if there's interest; if not, I'll go with
the warning message as suggested.

Thanks,
Yodel

[1] https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html (Peter)
[2] 
https://gitlab.com/qemu-project/qemu/-/blob/master/configure?ref_type=heads#L302-315

-- >8 --

diff --git a/configure b/configure
index 5e114acea2..861723b0a4 100755
--- a/configure
+++ b/configure
@@ -245,6 +245,12 @@ for opt do
    ;;
    --wasm64-32bit-address-limit) wasm64_memory64="2"
    ;;
+  --enable-asan) use_sanitizers="yes"
+  ;;
+  --enable-tsan) use_sanitizers="yes"
+  ;;
+  --enable-safe-stack) use_sanitizers="yes"
+  ;;
    esac
  done

@@ -286,15 +292,25 @@ static="no"
  # Preferred compiler:
  #  ${CC} (if set)
  #  ${cross_prefix}gcc (if cross-prefix specified)
-#  system compiler
+#  clang if sanitizer requested, otherwise system compiler
  if test -z "${CC}${cross_prefix}"; then
-  cc="cc"
+  if test "$use_sanitizers" = "yes" && has clang; then
+    cc=clang
+    echo "Sanitizer requested: setting cc to clang"
+  else
+    cc="cc"
+  fi
  else
    cc="${CC-${cross_prefix}gcc}"
  fi

  if test -z "${CXX}${cross_prefix}"; then
-  cxx="c++"
+  if test "$use_sanitizers" = "yes" && has clang; then
+    cxx=clang++
+    echo "Sanitizer requested: setting cxx to clang++"
+  else
+    cxx="c++"
+  fi
  else
    cxx="${CXX-${cross_prefix}g++}"
  fi



  parent reply	other threads:[~2026-03-04  1:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-03  1:20 [RFC PATCH v2] configure: Use clang for sanitizer builds or disable Werror Yodel Eldar
2026-03-03  8:41 ` Daniel P. Berrangé
2026-03-03  9:36   ` Peter Maydell
2026-03-03 10:01     ` Daniel P. Berrangé
2026-03-03 10:05       ` Peter Maydell
2026-03-03  8:49 ` Thomas Huth
2026-03-04  1:08 ` Yodel Eldar [this message]
2026-03-04  7:38   ` Thomas Huth
2026-03-04 23:14     ` Yodel Eldar
2026-03-05 10:18       ` Daniel P. Berrangé
2026-03-05 10:47         ` Peter Maydell
2026-03-04 10:09   ` Daniel P. Berrangé

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=f999293f-c424-497a-8bca-2634bcd68108@yodel.dev \
    --to=yodel.eldar@yodel.dev \
    --cc=alex.bennee@linaro.org \
    --cc=berrange@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.