Re: [PATCH 5/5] Makefile: disable unaligned loads with UBSan

2017-07-16 Thread René Scharfe 

Am 16.07.2017 um 12:17 schrieb Jeff King:

On Sat, Jul 15, 2017 at 07:18:56PM +0200, René Scharfe wrote:


-- >8 --
Subject: [PATCH] Makefile: allow combining UBSan with other sanitizers

Multiple sanitizers can be specified as a comma-separated list.  Set
the flag NO_UNALIGNED_LOADS even if UndefinedBehaviorSanitizer is not
the only sanitizer to build with.


Nice, I didn't know you could do comma-separation here. ;)

I always just built the various sanitizers separately since I was
testing whether each one worked. But if we can get UBSan to build
cleanly, I agree that "make SANITIZE=address,undefined test" would be a
nice thing to run periodically.


There are *some* restrictions: You can only use one of address, memory
and thread, as mentioned here:

http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation

Combining ASan and UBSan works fine.

René

Re: [PATCH 5/5] Makefile: disable unaligned loads with UBSan

2017-07-16 Thread Jeff King 
On Sat, Jul 15, 2017 at 07:18:56PM +0200, René Scharfe wrote:

> -- >8 --
> Subject: [PATCH] Makefile: allow combining UBSan with other sanitizers
> 
> Multiple sanitizers can be specified as a comma-separated list.  Set
> the flag NO_UNALIGNED_LOADS even if UndefinedBehaviorSanitizer is not
> the only sanitizer to build with.

Nice, I didn't know you could do comma-separation here. ;)

I always just built the various sanitizers separately since I was
testing whether each one worked. But if we can get UBSan to build
cleanly, I agree that "make SANITIZE=address,undefined test" would be a
nice thing to run periodically.

> diff --git a/Makefile b/Makefile
> index ba4359ef8d..9b98535a04 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1022,10 +1022,15 @@ ifdef DEVELOPER
>  CFLAGS += $(DEVELOPER_CFLAGS)
>  endif
>  
> +comma := ,
> +empty :=
> +space := $(empty) $(empty)
> +
>  ifdef SANITIZE
> +SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
>  BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  BASIC_CFLAGS += -fno-omit-frame-pointer
> -ifeq ($(SANITIZE),undefined)
> +ifneq ($(filter undefined,$(SANITIZERS)),)

The implementation is convoluted in the way that "make" usually forces
us into. ;)

I would have written it as:

  ifeq ($findstring $(SANITIZERS), undefined), undefined)

but I think your version:

  1. Isn't fooled by superstrings like a potential undefined-foo
 sanitizer.

  2. Handles SANITIZE=undefined,undefined correctly.

Neither are expected, but it doesn't hurt to be a bit more robust.

-Peff

Re: [PATCH 5/5] Makefile: disable unaligned loads with UBSan

2017-07-15 Thread René Scharfe 
Am 10.07.2017 um 15:24 schrieb Jeff King:
> The undefined behavior sanitizer complains about unaligned
> loads, even if they're OK for a particular platform in
> practice. It's possible that they _are_ a problem, of
> course, but since it's a known tradeoff the UBSan errors are
> just noise.
> 
> Let's quiet it automatically by building with
> NO_UNALIGNED_LOADS when SANITIZE=undefined is in use.
> 
> Signed-off-by: Jeff King 
> ---
>   Makefile | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index cc03b3c95..3c341b2a6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1015,6 +1015,9 @@ endif
>   ifdef SANITIZE
>   BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>   BASIC_CFLAGS += -fno-omit-frame-pointer
> +ifeq ($(SANITIZE),undefined)
> +BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
> +endif
>   endif
>   
>   ifndef sysconfdir

Nice, but let's be even nicer!

-- >8 --
Subject: [PATCH] Makefile: allow combining UBSan with other sanitizers

Multiple sanitizers can be specified as a comma-separated list.  Set
the flag NO_UNALIGNED_LOADS even if UndefinedBehaviorSanitizer is not
the only sanitizer to build with.

Signed-off-by: Rene Scharfe 
---
 Makefile | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index ba4359ef8d..9b98535a04 100644
--- a/Makefile
+++ b/Makefile
@@ -1022,10 +1022,15 @@ ifdef DEVELOPER
 CFLAGS += $(DEVELOPER_CFLAGS)
 endif
 
+comma := ,
+empty :=
+space := $(empty) $(empty)
+
 ifdef SANITIZE
+SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))
 BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
 BASIC_CFLAGS += -fno-omit-frame-pointer
-ifeq ($(SANITIZE),undefined)
+ifneq ($(filter undefined,$(SANITIZERS)),)
 BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
 endif
 endif
-- 
2.13.3