[PATCH] tests/docker: only enable ubsan for test-clang

[PATCH] tests/docker: only enable ubsan for test-clang

[Paolo Bonzini]

-fsanitize=undefined is not the same thing as --enable-sanitizers.  After
commit 47c823e ("tests/docker: add sanitizers back to clang build", 2019-09-11)
test-clang is almost duplicating the asan (test-debug) test, so
partly revert commit 47c823e5b while leaving ubsan enabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/docker/test-clang | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/docker/test-clang b/tests/docker/test-clang
index db9e697..8c51ead 100755
--- a/tests/docker/test-clang
+++ b/tests/docker/test-clang
@@ -17,7 +17,9 @@ requires clang
 
 cd "$BUILD_DIR"
 
-OPTS="--cxx=clang++ --cc=clang --host-cc=clang --enable-sanitizers"
+OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
+OPTS="$OPTS --extra-cflags=-fsanitize=undefined \
+    --extra-cflags=-fno-sanitize=float-divide-by-zero"
 build_qemu $OPTS
 check_qemu
 install_qemu
-- 
1.8.3.1

Re: [PATCH] tests/docker: only enable ubsan for test-clang

[John Snow]

On 10/1/19 10:14 AM, Paolo Bonzini wrote:
> -fsanitize=undefined is not the same thing as --enable-sanitizers.  After
> commit 47c823e ("tests/docker: add sanitizers back to clang build", 2019-09-11)
> test-clang is almost duplicating the asan (test-debug) test, so
> partly revert commit 47c823e5b while leaving ubsan enabled.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I got confused by this:

```
  if compile_prog "$CPU_CFLAGS -Werror -fsanitize=undefined" ""; then
      have_ubsan=yes
  fi

```

it looked quite like --enable-sanitizers was indeed the same as
-fsanitize=undefined; or at least was a superset of such.

I suppose the argument you're making here is that we *only* want ubsan
for test-clang?

(I guess so, since test-debug also stipulates that clang be used; but
enables debug.)

In this case, is there a use for test-clang at all, actually?

--js


> ---
>  tests/docker/test-clang | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/test-clang b/tests/docker/test-clang
> index db9e697..8c51ead 100755
> --- a/tests/docker/test-clang
> +++ b/tests/docker/test-clang
> @@ -17,7 +17,9 @@ requires clang
>  
>  cd "$BUILD_DIR"
>  
> -OPTS="--cxx=clang++ --cc=clang --host-cc=clang --enable-sanitizers"
> +OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
> +OPTS="$OPTS --extra-cflags=-fsanitize=undefined \
> +    --extra-cflags=-fno-sanitize=float-divide-by-zero"
>  build_qemu $OPTS
>  check_qemu
>  install_qemu
> 

-- 
—js

Re: [PATCH] tests/docker: only enable ubsan for test-clang

[Paolo Bonzini]

On 01/10/19 19:59, John Snow wrote:
> 
> it looked quite like --enable-sanitizers was indeed the same as
> -fsanitize=undefined; or at least was a superset of such.
> 
> I suppose the argument you're making here is that we *only* want ubsan
> for test-clang?
> 
> (I guess so, since test-debug also stipulates that clang be used; but
> enables debug.)
> 
> In this case, is there a use for test-clang at all, actually?

Yes, test-clang checks that clang-specific warnings do not fire.
Enabling ubsan isn't particularly useful in this respect, but it's okay
for me to keep it.

Paolo

Re: [PATCH] tests/docker: only enable ubsan for test-clang

[John Snow]

On 10/6/19 12:06 PM, Paolo Bonzini wrote:
> On 01/10/19 19:59, John Snow wrote:
>>
>> it looked quite like --enable-sanitizers was indeed the same as
>> -fsanitize=undefined; or at least was a superset of such.
>>
>> I suppose the argument you're making here is that we *only* want ubsan
>> for test-clang?
>>
>> (I guess so, since test-debug also stipulates that clang be used; but
>> enables debug.)
>>
>> In this case, is there a use for test-clang at all, actually?
> 
> Yes, test-clang checks that clang-specific warnings do not fire.
> Enabling ubsan isn't particularly useful in this respect, but it's okay
> for me to keep it.
> 
> Paolo
> 

If it's a proper subset of test-debug, let's just drop it entirely and
leave it to be a "normal" clang invocation.

(Oh, it was already pulled. well, whatever.)

--js