patchFix Solaris Workshop Compiler issues

Hello,

please find below a patch that solves an error when compiling with the original Sun Solaris Compiler. When compiling out of the box, the following happens:

     CC diff-delta.o
"diff-delta.c", line 314: identifier redeclared: create_delta
 	current : function(pointer to const struct delta_index {unsigned long memsize, pointer to const void src_buf, unsigned long src_size, unsigned int hash_mask, array[-1] of pointer to struct index_entry {..} hash}, pointer to const void, unsigned long, pointer to unsigned long, unsigned long) returning pointer to void
 	previous: function(pointer to const struct delta_index {unsigned long memsize, pointer to const void src_buf, unsigned long src_size, unsigned int hash_mask, array[-1] of pointer to struct index_entry {..} hash}, pointer to const void, unsigned long, pointer to unsigned long, unsigned long) returning pointer to void : "delta.h", line 44
cc: acomp failed for diff-delta.c
make: *** [diff-delta.o] Error 2

This is because 'struct delta_index' is declared with no size in delta.h and with size in diff-delta.c which does not fit.

When the struct definition is done in the header file as one would normally expect, everything compiles ok with exception of a mkdtemp()-issue which somebody else already took care of on this list.

Best regards

Guido

Guido Ostkamp, Wed, Nov 14, 2007 21:31:13 +0100:

The both prototypes listed are *exactly* the same. And both are wrong. Looks like you're dealing with typically broken Sun compiler.

Try defining const to nothing or removing it from this prototype. I suspect the thing is just so old and broken that it does not even know anything of const.

Huh?! Ever heard of forward declaration?

Alex Riesen <raa.lkml@gmail.com> writes:

We are not the first people who pass around a pointer to an opaque struct in the API to hide away the implementation. It would be surprising if the Workshop Compiler chokes on this and not other projects.

On Wed, 14 Nov 2007, Junio C Hamano wrote:

You got the original error report from Sun's compiler included in my earlier email. This happens with at least Sun Forte 6.1 (Solaris 8) and Sun Workshop 11 (Solaris 10), IIRC.

The function declarations regarding create_delta() in delta.h and diff-delta.c are identical with respect to the type names of the parameter (only some internal names e.g. like 'buf' vs. 'trg_buf' are slightly different, but this has no effect).

The main difference is that the 'struct delta_index' is opaque in delta.h and non-opaque in diff-delta.c; the patch clearly shows it solves the error. So we've got a solution.

If you feel we could try something else, please let me know and I'll check it out.

Please keep me on CC, as I'm not subscribed to the list, thanks.

Regards

Guido

Guido Ostkamp, Thu, Nov 15, 2007 00:21:55 +0100:

It is not the solution. How do you think the rest of Git compiles?

Yes. Read the mail I sent before:

    Try removing the "const". Looks like that compiler is too stupid
    to understand it.

On 2007.11.15 00:28:09 +0100, Alex Riesen wrote:

No, just tried with cc: Sun C 5.7 Patch 117837-06 2005/10/05

It's the "struct hack", ie. the incomplete array at the end of delta_index. Still looking for a fix/workaround.

Björn

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

Do you mean the "FLEX_ARRAY" thing?

You can ask for FLEX_ARRAY from the command line of your "make" process.

There is this thing in git-compat-util.h

        #ifndef FLEX_ARRAY
        #if defined(__GNUC__) && (__GNUC__ < 3)
        #define FLEX_ARRAY 0
        #else
        #define FLEX_ARRAY /* empty */
        #endif
        #endif

The sources are written this way:

	struct foo {
        	... other members ...
                char last_member_that_is_flexible[FLEX_ARRAY];
	};

For older gcc, because we know about its lack of support, the above turns into:

	struct foo {
        	... other members ...
                char last_member_that_is_flexible[0];
        }

But for recent enough compilers that grok the "flexible array members", the above expands to:

	struct foo {
        	... other members ...
                char last_member_that_is_flexible[];
        }

Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?

On 2007.11.14 16:30:08 -0800, Junio C Hamano wrote:

Actually, I just created a test-case remotely on a Solaris box in my university (see below) and didn't compile the actual git code. With the FAM, cc complains about a redeclared identifier, with a zero-sized array, it complains that an array cannot be zero-sized...

Seems to be a known bug in Sun Studio 10: http://forum.java.sun.com/thread.jspa?threadID=5071896&messageID=9263771

Björn

#include <stdio.h>

struct foo; void bar(const struct foo *, unsigned long);

struct bla {
	unsigned long foo;
};

struct foo {
	unsigned long val;
	struct bla *blas[];
};

void bar(const struct foo *foo, unsigned long val)
{
	printf("%lu %lu\n", foo->val, val);
}

int main()
{
	struct foo foo;
	foo.val = 10;
	bar(&foo, 20);

	return 0;
}

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

I think you can pass -DFLEX_ARRAY=1 as a workaround. It would waste one array member in a flexible structure but that is better than compiler choking.

On 2007.11.14 16:46:20 -0800, Junio C Hamano wrote:

Yeah, that at least compiles (didn't do any further tests), forgot to say that in the last email.

Björn

Some versions of SUN's cc have a bug that causes them to complain about a redeclared identifier when you use a function declaration that takes a struct with a FAM and this struct has only been declared but not yet defined.

IOW, this will fail:
	struct foo;

	void bar(struct foo *);

	struct foo {
		int v;
		long *a[];
	};

	void bar(struct foo *foo) {} // SUN cc bug strikes here

So when we detect a SUN cc, we use an array size of 1 to workaround that bug.

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
Guido, could you please test this patch?

I have no clue which versions of SUN's cc are affected, so I simply enabled the workaround for all versions. Someone with more knowledge about that should probably limit the check to only do that for the broken versions.

 git-compat-util.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

On Thu, 15 Nov 2007, Björn Steinbrink wrote:

I tested it and the file in question compiled ok for the two versions of Sun Forte and Workshop compilers that I have available.

I could also link & run the main git binary after applying my own mkdtemp() patch.

I hope your patch will get officially included.

Regards

Guido

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

Are there problems with the implementation in compat/ directory, we ship specifically to help platforms without mkdtemp()?

On Thu, 15 Nov 2007, Junio C Hamano wrote:

The Git version that I used for testing at office did not yet include your compat fix or did not activate it automatically.

I shall check this out tomorrow and let you know. Sorry, it's already late here in Germany (23:27h) - I need to get some sleep ;-)

Regards

Guido

On Thu, 15 Nov 2007, Junio C Hamano wrote:

I checked again and the answer is 'yes'. The reason is trivial - for Solaris 10 the workaround is not activated and my version of Solaris 10 (Sparc) has no mkdtemp() in libc.so.

The following patch should fix this:

Activate mkdtemp() workaround for Solaris 10.

Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  Makefile |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

Thanks.

This makes me wonder if treating it just like strcasestr() might be simpler. Could folks with access to Solaris boxes of different vintages please see if the attached patch makes sense?

Can we also unify UNSETENV, SETENV, C99_FORMAT and STRTOUMAX, by the way?

On Fri, 16 Nov 2007, Junio C Hamano wrote:

I think the patch makes sense as neither Solaris 8, 9 nor 10 supports mkdtemp().

No.

I've just checked on our Solaris Sparc systems, and found that the C-library provides unsetenv(), setenv() and strtoumax() beginning with Solaris 10; also the 'man sprintf' page mentions the 'z' and 't' specifiers for printf (which is what is behind C99_FORMAT) only beginning with Solaris 10.

So workarounds are needed for all 4 cases for Solaris 8 and 9 but not 10.

Regards

Guido

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

Thank you very much for detailed information.

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

This feels a bit too narrow and too broad at the same time, doesn't it?

As I suspect there are other compilers that do not implement flexible array members (so you cannot use "member[]") nor older gcc extension of zero sized member (so you cannot use "member[0]" either), this checking specifically for Sun is too narrow.

On the other hand, as you said, this is too broad, because not everybody may be using the SUN compiler on Sun, nor the version that does not understand flexible array members.

But being broad should always be safer, albeit a bit wasteful.

How about doing it this way?

  # ifndef FLEX_ARRAY
  #   if defined(__GNUC__)
  #     if (__GNUC__ < 3)
  #       define FLEX_ARRAY 0
  #     else
  #       define FLEX_ARRAY /* empty */
  #     endif
  #   else
        /* more cases we know we can use 0 or empty can come here */
  #   endif
  # endif

  /* if still undefined, default to the safe, old fashioned way */
  # ifndef FLEX_ARRAY
  #   define FLEX_ARRAY 1
  # endif

The basic idea is:

 * The user (from Makefile command line, config.mak, or you
   could add autoconf test) can pass -DFLEX_ARRAY=... to specify
   exactly what should happen;

 * Otherwise, if we happen to know for sure that we can use "0"
   or "/* empty */" with the compiler, we define FLEX_ARRAY;
   currently we know such things for gcc.

 * For everybody else, we use safer default of "1".  IOW, if you
   know your compiler does not grok "/* empty */" nor "0", you
   do not have to do anything special but use the default case
   as everybody else.

On 2007.11.15 20:58:55 -0800, Junio C Hamano wrote:

Yeah, looks a bit smarter.

Eeeek! He said autoconf! Waaah! ... Ehrm, sorry where was I?

Yep, definitely better than my patch.

Björn

Hello Junio,

On Thu, 15 Nov 2007, Junio C Hamano wrote:

it looks ok on Solaris. I assembled the following patch from your posting, could you please include it?

Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  git-compat-util.h |   11 ++++++++++-
  1 files changed, 10 insertions(+), 1 deletions(-)

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

I knew it would work on Solaris with gcc and cc that do not understand flexible array members, but I am a bit worried about other environments, where flexible array members are properly supported. They've been happily using member[] but with the patch they suddenly start wasting a cell.

But we should do this sooner rather than later to find out any breakage, and give people on platforms with a cc that supports flexible array members and care about wasted memory enough time to send patches to support their compiler in the way similar to how gcc is supported.

But I cannot use your message with whitespace-broken patch (note "format=flawed") and insufficient commit log message, which means I have to do this myself. Not tonight...

Hello Junio,

On Tue, 20 Nov 2007, Junio C Hamano wrote:

sorry for the whitespace-issues. I have attached the patch again with improved log message and will turn off format-flawed for this email.

Please let me know if this one is ok and feel free to fix it.

Log message starts here:

Fix "identifier redeclared" compilation error with SUN cc.

The problem is caused by incomplete arrays like

   struct foo {
     ...
     char last_member_that_is_flexible[];
   }

which cannot be handled by certain compilers. The solution is to change the last member to either

     char last_member_that_is_flexible[0]

or

     char last_member_that_is_flexible[1]

as required. Currently GNU CC can handle [] format for version 3 and later. Earlier versions need [0]. Non-GNU compiler use the safe form [1].

Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  git-compat-util.h |   11 ++++++++++-
  1 files changed, 10 insertions(+), 1 deletions(-)

only for the list: I noticed another whitespace problem and have meanwhile sent the patch to Junio as true attachment of a private email because I do not know whether attachments are accepted on this list (most likely they are not). Sorry for the inconvenience.

Regards

Guido

Hello!

Do we really want to use empty FLEX_ARRAY only for a new gcc? Shouldn't we test for C99 instead (__STDC_VERSION__ >= 199901L) and only if it isn't C99, choose between 0 and 1 depending on gccness of the compiler?

				Have a nice fortnight

Martin Mares <mj@ucw.cz> writes:

How about doing it this way?

-- >8 -- [PATCH] git-compat-util.h: auto-adjust to compiler support of FLEX_ARRAY a bit better

When declaring a structure with a flexible array member, instead of defaulting to the c99 syntax for non-gnu compilers (which burned people with older compilers), default to the traditional and more portable "member[1]; /* more */" syntax.

At the same time, other c99 compilers should be able to take advantage of the modern syntax to flexible array members without being gcc. Check __STDC_VERSION__ for that.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

Hello!

Yes, this looks perfect.

				Have a nice fortnight

On Wed, 14 Nov 2007, Junio C Hamano wrote:

Actually, for old pre-C99 compilers, you're probably better off using -DFLEX_ARRAY=1, since a zero-sized array could be considered bogus by some.

			Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Is that supposed to work? I would have thought that the only options would be empty and 0. I am pretty sure I have seen size calculations in the deltifying code that would break badly using FLEX_ARRAY=1. So _IFF_ -DFLEX_ARRAY=1 is supposed to be necessary for some compilers, I could try seeing whether I find those locations again.

On Thu, 15 Nov 2007, David Kastrup wrote:

I think it should work, even though some things allocations will end up being a bit too large (ie anything that uses "sizeof()" will have that one unnecessary entry)

But no, I didn't actually test it.

		Linus

David Kastrup <dak@gnu.org> writes:

I do recall that I received a patch with an explicit member elem[1] that is in fact used as a flexible array, foolishly converted it to use FLEX_ARRAY and saw it mysteriously fail, and realized what it was doing and reverted my changes, and applied the patch as received. IIRC it all happened before I pushed the results out. I unfortunately do not recall which area the patch was about.