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

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

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 <peff@peff.net> 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 ./t0000-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