diff mbox series

[3/3] build-sys: enable sanitizers by default with --enable-debug

Message ID 20180208162343.30809-3-marcandre.lureau@redhat.com
State New
Headers show
Series [1/3] build-sys: remove useless extra*flags variables | expand

Commit Message

Marc-André Lureau Feb. 8, 2018, 4:23 p.m. UTC
The original commit 247724cb302af5d70c8853154b640dfabf2bbb56 was meant
to enable sanitizers by default when --enable-debug, but failed
because of a gcc static linking bug. Try to enable it back now that
there is a stronger check.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini Feb. 8, 2018, 5:46 p.m. UTC | #1
On 08/02/2018 17:23, Marc-André Lureau wrote:
> The original commit 247724cb302af5d70c8853154b640dfabf2bbb56 was meant
> to enable sanitizers by default when --enable-debug, but failed
> because of a gcc static linking bug. Try to enable it back now that
> there is a stronger check.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I'm more afraid that there are quite a few reports from sanitizers.  I
wonder if that makes --enable-debug unusable; as a non-user of
--enable-debug I'm a bit wary of pushing this patch.

I've queued 1 and 2 though.

Paolo

> ---
>  configure | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index beb0de6a77..700ff35383 100755
> --- a/configure
> +++ b/configure
> @@ -355,7 +355,7 @@ rdma=""
>  gprof="no"
>  debug_tcg="no"
>  debug="no"
> -sanitizers="no"
> +sanitizers=""
>  fortify_source=""
>  strip_opt="yes"
>  tcg_interpreter="no"
> @@ -5262,6 +5262,11 @@ have_ubsan=no
>  have_asan_iface_h=no
>  have_asan_iface_fiber=no
>  
> +# enable sanitizers by default if --enable-debug
> +if test "$sanitizers" = "" -a "$debug" = "yes"; then
> +  sanitizers=yes
> +fi
> +
>  if test "$sanitizers" = "yes" ; then
>    if compile_prog "$CPU_CFLAGS -Werror -fsanitize=address" ""; then
>        have_asan=yes
>
Marc-André Lureau Feb. 8, 2018, 7:03 p.m. UTC | #2
Hi

On Thu, Feb 8, 2018 at 6:46 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 08/02/2018 17:23, Marc-André Lureau wrote:
>> The original commit 247724cb302af5d70c8853154b640dfabf2bbb56 was meant
>> to enable sanitizers by default when --enable-debug, but failed
>> because of a gcc static linking bug. Try to enable it back now that
>> there is a stronger check.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I'm more afraid that there are quite a few reports from sanitizers.  I
> wonder if that makes --enable-debug unusable; as a non-user of
> --enable-debug I'm a bit wary of pushing this patch.

I understand the concern, but at the same time, people should care
about fixing those. If they want to keep ignoring them (for bad
reasons), they can --disable-sanitizers. At least, I would want to
reach a point where no ASAN regression get introduced in x86 target.
It would be nice if patchew warn on new ASAN reports for example.

>
> I've queued 1 and 2 though.
>
> Paolo
>
>> ---
>>  configure | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index beb0de6a77..700ff35383 100755
>> --- a/configure
>> +++ b/configure
>> @@ -355,7 +355,7 @@ rdma=""
>>  gprof="no"
>>  debug_tcg="no"
>>  debug="no"
>> -sanitizers="no"
>> +sanitizers=""
>>  fortify_source=""
>>  strip_opt="yes"
>>  tcg_interpreter="no"
>> @@ -5262,6 +5262,11 @@ have_ubsan=no
>>  have_asan_iface_h=no
>>  have_asan_iface_fiber=no
>>
>> +# enable sanitizers by default if --enable-debug
>> +if test "$sanitizers" = "" -a "$debug" = "yes"; then
>> +  sanitizers=yes
>> +fi
>> +
>>  if test "$sanitizers" = "yes" ; then
>>    if compile_prog "$CPU_CFLAGS -Werror -fsanitize=address" ""; then
>>        have_asan=yes
>>
>
>
Fam Zheng Feb. 9, 2018, 2:24 a.m. UTC | #3
On Thu, 02/08 20:03, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Feb 8, 2018 at 6:46 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 08/02/2018 17:23, Marc-André Lureau wrote:
> >> The original commit 247724cb302af5d70c8853154b640dfabf2bbb56 was meant
> >> to enable sanitizers by default when --enable-debug, but failed
> >> because of a gcc static linking bug. Try to enable it back now that
> >> there is a stronger check.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > I'm more afraid that there are quite a few reports from sanitizers.  I
> > wonder if that makes --enable-debug unusable; as a non-user of
> > --enable-debug I'm a bit wary of pushing this patch.
> 
> I understand the concern, but at the same time, people should care
> about fixing those. If they want to keep ignoring them (for bad
> reasons), they can --disable-sanitizers. At least, I would want to
> reach a point where no ASAN regression get introduced in x86 target.
> It would be nice if patchew warn on new ASAN reports for example.

Could you write a docker test for that?

Fam
diff mbox series

Patch

diff --git a/configure b/configure
index beb0de6a77..700ff35383 100755
--- a/configure
+++ b/configure
@@ -355,7 +355,7 @@  rdma=""
 gprof="no"
 debug_tcg="no"
 debug="no"
-sanitizers="no"
+sanitizers=""
 fortify_source=""
 strip_opt="yes"
 tcg_interpreter="no"
@@ -5262,6 +5262,11 @@  have_ubsan=no
 have_asan_iface_h=no
 have_asan_iface_fiber=no
 
+# enable sanitizers by default if --enable-debug
+if test "$sanitizers" = "" -a "$debug" = "yes"; then
+  sanitizers=yes
+fi
+
 if test "$sanitizers" = "yes" ; then
   if compile_prog "$CPU_CFLAGS -Werror -fsanitize=address" ""; then
       have_asan=yes