Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Jeff King]

On Mon, Jul 10, 2017 at 09:02:24PM +0100, Ramsay Jones wrote:

> After a quick look at the ./t-basic.sh test, I managed to get
> the test to complete (with 15 tests failing), with the following
> patch applied:
> 
> -- >8 --
> diff --git a/Makefile b/Makefile
> index 3c341b2a6..8e6433738 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1016,7 +1016,7 @@ ifdef SANITIZE
>  BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  BASIC_CFLAGS += -fno-omit-frame-pointer
>  ifeq ($(SANITIZE),undefined)
> -BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
> +BASIC_CFLAGS += -DNO_UNALIGNED_LOADS -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
>  endif

Thanks, I forgot to mention SHA1DC. When I had originally tested with
"undefined", it was before we had SHA1DC. I hacked around it earlier
today by just using OPENSSL_SHA1. ;)

I agree if we can ask it to avoid unaligned access that is even better.

> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index 25eded139..3baddc636 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -118,6 +118,10 @@
>  #define SHA1DC_ALLOW_UNALIGNED_ACCESS
>  #endif /*UNALIGNMENT DETECTION*/
>  
> +#if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) && 
> defined(SHA1DC_FORCE_ALIGNED_ACCESS)
> +#undef SHA1DC_ALLOW_UNALIGNED_ACCESS
> +#endif

I think our current strategy is to avoid touching sha1.c as much as
possible. I think we'd prefer a patch to the upstream project to support
FORCE_ALIGNED_ACCESS (unfortunately I do not see a way to tweak it using
only external defines.

-Peff

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Ramsay Jones]

On 10/07/17 18:44, Jeff King wrote:
> On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:
> 
>> Jeff King  writes:
>>
>>> You can already build and test with ASan by doing:
>>>
>>>   make CFLAGS=-fsanitize=address test
>>>
>>> but there are a few slight annoyances:
>>>
>>>   1. It's a little long to type.
>>>
>>>   2. It override your CFLAGS completely. You'd probably
>>>  still want -O2, for instance.
>>>
>>>   3. It's a good idea to also turn off "recovery", which
>>>  lets the program keep running after a problem is
>>>  detected (with the intention of finding as many bugs as
>>>  possible in a given run). Since Git's test suite should
>>>  generally run without triggering any problems, it's
>>>  better to abort immediately and fail the test when we
>>>  do find an issue.
>>
>> Unfortunately I do not think Comparing between versions in
>> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
>> is not configurable for folks still with GCC 4.x series, and this
>> patch is not very useful unless you disable the recovery for the
>> purpose of running our tests as you said X-<.
> 
> I didn't actually dig into the history of gcc support at all. Back in
> the 4.x time-frame I tried using ASan and couldn't get it to work at
> all. I ended up just always building with clang (which from my
> mostly-ignorant view seems to to be the primary platform for ASan
> development).
> 
> Since this is an optional build that doesn't need to be available
> everywhere, I'd actually be fine with saying "just use clang". But as
> far as I can tell, gcc seems to work fine these days. I consider this
> mostly a best-effort tool.
> 
> I'm also not sure of the behavior without -fno-sanitize-recover. I think
> ASan may barf either way. The commit message for my config.mak from a
> year or two ago claims that the problem was actually with UBSan. It
> would be useful in the long run for that to work, too.

Just FYI, I had a quick look at this tonight. I applied your
patches to master, the tried 'make SANITIZE=address test', which
worked fine. I then tried 'make SANITIZE=undefined test' and I had
to control+C it after nearly two hours on one test! ;-) (somewhere
in the t4xxx - unfortunately I overwrote the output file without
thinking).

[BTW I am on Linux Mint 18.2 x86_64, gcc version 5.4.0]

After a quick look at the ./t-basic.sh test, I managed to get
the test to complete (with 15 tests failing), with the following
patch applied:

-- >8 --
diff --git a/Makefile b/Makefile
index 3c341b2a6..8e6433738 100644
--- a/Makefile
+++ b/Makefile
@@ -1016,7 +1016,7 @@ ifdef SANITIZE
 BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
 BASIC_CFLAGS += -fno-omit-frame-pointer
 ifeq ($(SANITIZE),undefined)
-BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
+BASIC_CFLAGS += -DNO_UNALIGNED_LOADS -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 endif
 
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 25eded139..3baddc636 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -118,6 +118,10 @@
 #define SHA1DC_ALLOW_UNALIGNED_ACCESS
 #endif /*UNALIGNMENT DETECTION*/
 
+#if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) && 
defined(SHA1DC_FORCE_ALIGNED_ACCESS)
+#undef SHA1DC_ALLOW_UNALIGNED_ACCESS
+#endif
+
 
 #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n
 #define rotate_left(x,n)  (((x)<<(n))|((x)>>(32-(n


Hmm, hopefully that is not whitespace damaged.

ATB,
Ramsay Jones

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > You can already build and test with ASan by doing:
>> >
>> >   make CFLAGS=-fsanitize=address test
>> >
>> > but there are a few slight annoyances:
>> >
>> >   1. It's a little long to type.
>> >
>> >   2. It override your CFLAGS completely. You'd probably
>> >  still want -O2, for instance.
>> >
>> >   3. It's a good idea to also turn off "recovery", which
>> >  lets the program keep running after a problem is
>> >  detected (with the intention of finding as many bugs as
>> >  possible in a given run). Since Git's test suite should
>> >  generally run without triggering any problems, it's
>> >  better to abort immediately and fail the test when we
>> >  do find an issue.
>> 
>> Unfortunately I do not think Comparing between versions in
>> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
>> is not configurable for folks still with GCC 4.x series, and this
>> patch is not very useful unless you disable the recovery for the
>> purpose of running our tests as you said X-<.
>
> I didn't actually dig into the history of gcc support at all. Back in
> the 4.x time-frame I tried using ASan and couldn't get it to work at
> all. I ended up just always building with clang (which from my
> mostly-ignorant view seems to to be the primary platform for ASan
> development).
>
> Since this is an optional build that doesn't need to be available
> everywhere, I'd actually be fine with saying "just use clang". But as
> far as I can tell, gcc seems to work fine these days. I consider this
> mostly a best-effort tool.
>
> I'm also not sure of the behavior without -fno-sanitize-recover. I think
> ASan may barf either way. The commit message for my config.mak from a
> year or two ago claims that the problem was actually with UBSan. It
> would be useful in the long run for that to work, too.

Yes.  I'd agree with all of the above.  While copyediting my
response, I somehow ended up removing one paragraph before that
"Unfortunately" by accident X-<, but the paragraph said essentially
the same "this is optional so it is a strict improvement, and I do
agree recovery must be disabled to be useful in our context".

Sorry for a possible confusion.

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Jeff King]

On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > You can already build and test with ASan by doing:
> >
> >   make CFLAGS=-fsanitize=address test
> >
> > but there are a few slight annoyances:
> >
> >   1. It's a little long to type.
> >
> >   2. It override your CFLAGS completely. You'd probably
> >  still want -O2, for instance.
> >
> >   3. It's a good idea to also turn off "recovery", which
> >  lets the program keep running after a problem is
> >  detected (with the intention of finding as many bugs as
> >  possible in a given run). Since Git's test suite should
> >  generally run without triggering any problems, it's
> >  better to abort immediately and fail the test when we
> >  do find an issue.
> 
> Unfortunately I do not think Comparing between versions in
> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
> is not configurable for folks still with GCC 4.x series, and this
> patch is not very useful unless you disable the recovery for the
> purpose of running our tests as you said X-<.

I didn't actually dig into the history of gcc support at all. Back in
the 4.x time-frame I tried using ASan and couldn't get it to work at
all. I ended up just always building with clang (which from my
mostly-ignorant view seems to to be the primary platform for ASan
development).

Since this is an optional build that doesn't need to be available
everywhere, I'd actually be fine with saying "just use clang". But as
far as I can tell, gcc seems to work fine these days. I consider this
mostly a best-effort tool.

I'm also not sure of the behavior without -fno-sanitize-recover. I think
ASan may barf either way. The commit message for my config.mak from a
year or two ago claims that the problem was actually with UBSan. It
would be useful in the long run for that to work, too.

-Peff

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Junio C Hamano]

Jeff King  writes:

> You can already build and test with ASan by doing:
>
>   make CFLAGS=-fsanitize=address test
>
> but there are a few slight annoyances:
>
>   1. It's a little long to type.
>
>   2. It override your CFLAGS completely. You'd probably
>  still want -O2, for instance.
>
>   3. It's a good idea to also turn off "recovery", which
>  lets the program keep running after a problem is
>  detected (with the intention of finding as many bugs as
>  possible in a given run). Since Git's test suite should
>  generally run without triggering any problems, it's
>  better to abort immediately and fail the test when we
>  do find an issue.

Unfortunately I do not think Comparing between versions in
https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
is not configurable for folks still with GCC 4.x series, and this
patch is not very useful unless you disable the recovery for the
purpose of running our tests as you said X-<.

> With this patch, all of that happens automatically when you
> run:
>
>   make SANITIZE=address test
>
> Signed-off-by: Jeff King 
> ---
>  Makefile | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 9c9c42f8f..59f6bdcd7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1012,6 +1012,10 @@ ifdef DEVELOPER
>  CFLAGS += $(DEVELOPER_CFLAGS)
>  endif
>  
> +ifdef SANITIZE
> +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
> +endif
> +
>  ifndef sysconfdir
>  ifeq ($(prefix),/usr)
>  sysconfdir = /etc