[PATCH 0/2] work around gcc 4.2.x compiler crash

[PATCH 0/2] work around gcc 4.2.x compiler crash

[Eric Sunshine]

This patch series works around a gcc 4.2.1 compiler crash reported on
Mac OS X[1] and FreeBSD[2] which is triggered by perfectly valid change
from 5b442c4 (tree-diff: catch integer overflow in combine_diff_path
allocation, 2016-02-19).

patch 1: sidestep the problem via a simple re-ordering of macro argument
    evaluation

patch 2: revert an earlier Mac OS X 10.6-specific work-around which is
    no longer needed

[1]: http://thread.gmane.org/gmane.comp.version-control.git/287486/focus=287706
[2]: http://thread.gmane.org/gmane.linux.kernel/2179363/focus=289357

Eric Sunshine (2):
  git-compat-util: st_add4: work around gcc 4.2.x compiler crash
  Revert "config.mak.uname: use clang for Mac OS X 10.6"

 config.mak.uname  | 3 ---
 git-compat-util.h | 4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

-- 
2.8.0.rc3.240.g104e649

[PATCH 1/2] git-compat-util: st_add4: work around gcc 4.2.x compiler crash

[Eric Sunshine]

Although changes by 5b442c4 (tree-diff: catch integer overflow in
combine_diff_path allocation, 2016-02-19) are perfectly valid, they
unfortunately trigger an internal compiler error in gcc 4.2.x:

    combine-diff.c: In function 'diff_tree_combined':
    combine-diff.c:1391: internal compiler error: Segmentation fault: 11

Experimentation reveals that changing st_add4()'s argument evaluation
order is sufficient to sidestep this problem.

Although st_add3() does not trigger the compiler bug, for style
consistency, change its argument evaluation order to match.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 git-compat-util.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index c07e0c1..4743954 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -715,8 +715,8 @@ static inline size_t st_add(size_t a, size_t b)
 		    (uintmax_t)a, (uintmax_t)b);
 	return a + b;
 }
-#define st_add3(a,b,c)   st_add((a),st_add((b),(c)))
-#define st_add4(a,b,c,d) st_add((a),st_add3((b),(c),(d)))
+#define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
+#define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
 
 static inline size_t st_mult(size_t a, size_t b)
 {
-- 
2.8.0.rc3.240.g104e649

Re: [PATCH 1/2] git-compat-util: st_add4: work around gcc 4.2.x compiler crash

[Jeff King]

On Mon, Mar 21, 2016 at 12:35:57AM -0400, Eric Sunshine wrote:

> Although changes by 5b442c4 (tree-diff: catch integer overflow in
> combine_diff_path allocation, 2016-02-19) are perfectly valid, they
> unfortunately trigger an internal compiler error in gcc 4.2.x:
> 
>     combine-diff.c: In function 'diff_tree_combined':
>     combine-diff.c:1391: internal compiler error: Segmentation fault: 11
> 
> Experimentation reveals that changing st_add4()'s argument evaluation
> order is sufficient to sidestep this problem.
> 
> Although st_add3() does not trigger the compiler bug, for style
> consistency, change its argument evaluation order to match.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  git-compat-util.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks for digging into this, the result is a really pleasant solution.
I think it's worth doing.

-Peff

Re: [PATCH 1/2] git-compat-util: st_add4: work around gcc 4.2.x compiler crash

[Christian Couder]

On Mon, Mar 21, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index c07e0c1..4743954 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -715,8 +715,8 @@ static inline size_t st_add(size_t a, size_t b)
>                     (uintmax_t)a, (uintmax_t)b);
>         return a + b;
>  }
> -#define st_add3(a,b,c)   st_add((a),st_add((b),(c)))
> -#define st_add4(a,b,c,d) st_add((a),st_add3((b),(c),(d)))
> +#define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
> +#define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))

Nit: maybe a comment around those lines would make sure that people do
not inadvertently change them back later.

Thanks,
Christian.

Re: [PATCH 1/2] git-compat-util: st_add4: work around gcc 4.2.x compiler crash

[Eric Sunshine]

On Mon, Mar 21, 2016 at 12:56 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Mar 21, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index c07e0c1..4743954 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -715,8 +715,8 @@ static inline size_t st_add(size_t a, size_t b)
>>                     (uintmax_t)a, (uintmax_t)b);
>>         return a + b;
>>  }
>> -#define st_add3(a,b,c)   st_add((a),st_add((b),(c)))
>> -#define st_add4(a,b,c,d) st_add((a),st_add3((b),(c),(d)))
>> +#define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
>> +#define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
>
> Nit: maybe a comment around those lines would make sure that people do
> not inadvertently change them back later.

Maybe, maybe not. I'm hesitant for the following reason.

Unless we determine the exact compiler bug and patch which fixed it,
we don't really have a good handle on what triggers this crash.
Consequently, even with a comment saying "don't change the code in
such and such a way", if someone ever does need to modify st_add4() in
some fashion, it's entirely possible that the modification will
trigger the crash again, even if the current evaluation order is kept
or only modified slightly. We just don't know.

So, such a comment doesn't strike me as having a lot of value, and
whatever value it does have wanes as this old compiler (gcc 4.2.1) and
these old platforms (Mac OS X 10.6 and FreeBSD 9.x) become less and
less relevant over time.

Re: [PATCH 1/2] git-compat-util: st_add4: work around gcc 4.2.x compiler crash

[Torsten Bögershausen]

On 2016-03-21 05.35, Eric Sunshine wrote:
>  }
> -#define st_add3(a,b,c)   st_add((a),st_add((b),(c)))
> -#define st_add4(a,b,c,d) st_add((a),st_add3((b),(c),(d)))
> +#define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
> +#define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
>  

That fix compiles on my test machine,
and passes the test suite.
thanks for digging.

[PATCH 2/2] Revert "config.mak.uname: use clang for Mac OS X 10.6"

[Eric Sunshine]

This reverts commit 7b6daf8d2fee1a9866b1d4eddbfaa5dbc42c5dbb.

Now that st_add4() has been patched to work around the gcc 4.2.x
compiler crash, revert the sledge-hammer approach of forcing Mac OS X
10.6 to unconditionally use 'clang' rather than the default compiler
(gcc).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

I verified that the work-around in patch 1/2 "fixes" the crash on
FreeBSD, and I'm pretty confident that it fixes it as well on Mac OS X
10.6 since it manifested identically on both platforms, but it would
still be nice to receive confirmation from Torsten.

config.mak.uname | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index 1139b44..fe8096f 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -103,9 +103,6 @@ ifeq ($(uname_S),Darwin)
 	ifeq ($(shell expr "$(uname_R)" : '[15]\.'),2)
 		NO_STRLCPY = YesPlease
 	endif
-	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -eq 10 && echo 1),1)
-		CC = clang
-	endif
 	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1),1)
 		HAVE_GETDELIM = YesPlease
 	endif
-- 
2.8.0.rc3.240.g104e649