FLEX_ARRAY=1 causes SIGSEGV on SPARC

FLEX_ARRAY=1 causes SIGSEGV on SPARC

[Shawn O. Pearce]

So today I discovered latest master will cause a SIGSEGV on
Solaris/SPARC if FLEX_ARRAY is defined to the default value of 1.

The issue is the (old) compiler I'm using from Sun doesn't pass
the tests defined in 8e9739914972419baad820e76b44d9720ed885c2 (aka
"git-compat-util.h: auto-adjust to compiler support of FLEX_ARRAY
a bit better") so we fallback to #define FLEX_ARRAY 1.

Then git-pack-objects finds something unaligned and segfaults.
It always segfaults.  During the deltification phase.  Probably it
was working on deltifying loose objects; the set I fed it was about
100 most recent objects so they were probably all loose.

I'll try to track it down tomorrow.  But the immediate workaround was
to just add '-DFLEX_ARRAY=/* empty */' to my CFLAGS and recompile
the world.  This compiler accepts the empty FLEX_ARRAY macro but
I'm not sure what feature test(s) would be necessary to make Git
able to automatically set that, seeing as how the tests defined in
8e97 are perfectly reasonable and didn't pass.

Yea, yea, shame on me for not testing Git since Nov 20th on this
system... I should have caught the failure sooner.  I'm buried in
non-Git work these days, but will try to come up with a reasonable
detection patch.  Maybe someone will beat me to it.  :-)

-- 
Shawn.

Re: FLEX_ARRAY=1 causes SIGSEGV on SPARC

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> I'll try to track it down tomorrow.  But the immediate workaround was
> to just add '-DFLEX_ARRAY=/* empty */' to my CFLAGS and recompile
> the world.  This compiler accepts the empty FLEX_ARRAY macro but
> I'm not sure what feature test(s) would be necessary to make Git
> able to automatically set that, seeing as how the tests defined in
> 8e97 are perfectly reasonable and didn't pass.
> ..., but will try to come up with a reasonable
> detection patch....

Actually I would be more worried about the breakage in FLEX_ARRAY=1 case
than misdetection.  Even if your compiler supports the flexible array
members, the fallback to FLEX_ARRAY=1 ought to work and you are seeing a
case where it doesn't.

[PATCH] diff-delta.c: make FLEX_ARRAY=1 work.

[David Kastrup]

I remarked previously that diff-delta.c does not work with FLEX_ARRAY=1.
Here is one attempt to change this.  It conceivably still suffers from
potential misalignment problems (which would likely need some union type
magic to avoid 100%), but at least the index calculation should not go
as horribly wrong as previously.

---
Junio C Hamano <gitster@pobox.com> writes:

> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> I'll try to track it down tomorrow.  But the immediate workaround was
>> to just add '-DFLEX_ARRAY=/* empty */' to my CFLAGS and recompile
>> the world.  This compiler accepts the empty FLEX_ARRAY macro but
>> I'm not sure what feature test(s) would be necessary to make Git
>> able to automatically set that, seeing as how the tests defined in
>> 8e97 are perfectly reasonable and didn't pass.
>> ..., but will try to come up with a reasonable
>> detection patch....
>
> Actually I would be more worried about the breakage in FLEX_ARRAY=1 case
> than misdetection.  Even if your compiler supports the flexible array
> members, the fallback to FLEX_ARRAY=1 ought to work and you are seeing a
> case where it doesn't.

 diff-delta.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff-delta.c b/diff-delta.c
index 9e440a9..099235c 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -247,7 +247,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
 	/* Now create the packed index in array form rather than
 	 * linked lists */
 
-	memsize = sizeof(*index)
+	memsize = (char *)&index->hash - (char *)index
 		+ sizeof(*packed_hash) * (hsize+1)
 		+ sizeof(*packed_entry) * entries;
 
@@ -264,7 +264,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
 	index->src_size = bufsize;
 	index->hash_mask = hmask;
 
-	mem = index + 1;
+	mem = &index->hash;
 	packed_hash = mem;
 	mem = packed_hash + (hsize+1);
 	packed_entry = mem;
-- 
1.5.3.6.995.ge8abd