threads / patch / 41765

patch, 2 partsgit-compat-util: st_add4: work around gcc 4.2.x compiler crash

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

## tl;dr

7 messages between Mar 21, 2016 and Mar 21, 2016. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Eric Sunshine· Mar 21, 2016, 04:35 UTC · lore

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

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

Eric Sunshine· Mar 21, 2016, 04:35 UTC · re: Eric Sunshine · lore

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(-)

Show changes to git-compat-util.h +2 −2

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

Jeff King· Mar 21, 2016, 04:43 UTC · re: Eric Sunshine · lore

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

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

Show 17 quoted lines

> 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

Christian Couder· Mar 21, 2016, 04:56 UTC · re: Eric Sunshine · lore

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

On Mon, Mar 21, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 13 quoted lines

>
> 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.

Eric Sunshine· Mar 21, 2016, 05:38 UTC · re: Christian Couder · lore

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

On Mon, Mar 21, 2016 at 12:56 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 17 quoted lines

> 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.

Torsten Bögershausen· Mar 21, 2016, 17:46 UTC · re: Eric Sunshine · lore

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

On 2016-03-21 05.35, Eric Sunshine wrote:

Show 6 quoted lines

>  }
> -#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.

Eric Sunshine· Mar 21, 2016, 04:35 UTC · re: Eric Sunshine · lore

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

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(-)

Show changes to config.mak.uname +0 −3

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