login
Search
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
diff --git a/delta.h b/delta.h
index 40ccf5a..06af9a7 100644
--- a/delta.h
+++ b/delta.h
@@ -1,8 +1,23 @@
#ifndef DELTA_H
#define DELTA_H
-/* opaque object for delta index */
-struct delta_index;
+struct index_entry {
+ const unsigned char *ptr;
+ unsigned int val;
+};
+
+struct unpacked_index_entry {
+ struct index_entry entry;
+ struct unpacked_index_entry *next;
+};
+
+struct delta_index {
+ unsigned long memsize;
+ const void *src_buf;
+ unsigned long src_size;
+ unsigned int hash_mask;
+ struct index_entry *hash[FLEX_ARRAY];
+};
/*
* create_delta_index: compute index data from given buffer
diff --git ...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 Huh?! Ever heard of forward declaration? -
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. -
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 -
Yes. Read the mail I sent before:
Try removing the "const". Looks like that compiler is too stupid
to understand it.
-
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 -
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?
-
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; } -
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. -
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(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index ede9408..c3ff4b4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -6,6 +6,8 @@
#ifndef FLEX_ARRAY
#if defined(__GNUC__) && (__GNUC__ < 3)
#define FLEX_ARRAY 0
+#elif defined(sun) || defined(__SUN__)
+#define FLEX_ARRAY 1
#else
#define FLEX_ARRAY /* empty */
#endif
--
1.5.3.5.643.g40e25
-
Are there problems with the implementation in compat/ directory, we ship specifically to help platforms without mkdtemp()? -
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 -
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(-) diff --git a/Makefile b/Makefile index e830bc7..9dc01df 100644 --- a/Makefile +++ b/Makefile @@ -431,6 +431,9 @@ ifeq ($(uname_S),SunOS) NO_C99_FORMAT = YesPlease NO_STRTOUMAX = YesPlease endif + ifeq ($(uname_R),5.10) + NO_MKDTEMP = YesPlease + endif INSTALL = ginstall TAR = gtar BASIC_CFLAGS += -D__EXTENSIONS__ -- 1.5.3.5.721.g039b -
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? diff --git a/Makefile b/Makefile index e830bc7..cabde81 100644 --- a/Makefile +++ b/Makefile @@ -412,26 +412,25 @@ endif ifeq ($(uname_S),SunOS) NEEDS_SOCKET = YesPlease NEEDS_NSL = YesPlease SHELL_PATH = /bin/bash NO_STRCASESTR = YesPlease NO_MEMMEM = YesPlease NO_HSTRERROR = YesPlease + NO_MKDTEMP = YesPlease ifeq ($(uname_R),5.8) NEEDS_LIBICONV = YesPlease NO_UNSETENV = YesPlease NO_SETENV = YesPlease - NO_MKDTEMP = YesPlease NO_C99_FORMAT = YesPlease NO_STRTOUMAX = YesPlease endif ifeq ($(uname_R),5.9) NO_UNSETENV = YesPlease NO_SETENV = YesPlease - NO_MKDTEMP = YesPlease NO_C99_FORMAT = YesPlease NO_STRTOUMAX = YesPlease endif INSTALL = ginstall TAR = gtar BASIC_CFLAGS += -D__EXTENSIONS__ endif -
I think the patch makes sense as neither Solaris 8, 9 nor 10 supports 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 -
Thank you very much for detailed information. -
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.
-
Hello Junio, 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(-) diff --git a/git-compat-util.h b/git-compat-util.h index 276a437..97759fd 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -4,11 +4,20 @@ #define _FILE_OFFSET_BITS 64 #ifndef FLEX_ARRAY -#if defined(__GNUC__) && (__GNUC__ < 3) +#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 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) -- 1.5.3.6.728.gea559 -
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,
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(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 276a437..97759fd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -4,11 +4,20 @@
#define _FILE_OFFSET_BITS 64
#ifndef FLEX_ARRAY
-#if defined(__GNUC__) && (__GNUC__ < 3)
+#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
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
--
1.5.3.6.728.gea559
-
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 -
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 `MJ' Mares <mj@ucw.cz> http://mj.ucw.cz/ Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth "First they ignore you. Then they laugh at you. Then they fight you. Then you win." -- Gandhi -
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(-) diff --git a/git-compat-util.h b/git-compat-util.h index 276a437..454d25e 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -4,10 +4,24 @@ #define _FILE_OFFSET_BITS 64 #ifndef FLEX_ARRAY -#if defined(__GNUC__) && (__GNUC__ < 3) -#define FLEX_ARRAY 0 -#else -#define FLEX_ARRAY /* empty */ +/* + * See if our compiler is known to support flexible array members. + */ +#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) +# define FLEX_ARRAY /* empty */ +#elif defined(__GNUC__) +# if (__GNUC__ >= 3) +# define FLEX_ARRAY /* empty */ +# else +# define FLEX_ARRAY 0 /* older GNU extension */ +# endif +#endif + +/* + * Otherwise, default to safer but a bit wasteful traditional style + */ +#ifndef FLEX_ARRAY +# define FLEX_ARRAY 1 #endif #endif -- 1.5.3.6.1797.g67f5d -
Yes, this looks perfect. Have a nice fortnight -- Martin `MJ' Mares <mj@ucw.cz> http://mj.ucw.cz/ Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth Hi! I'm a .signature virus! Copy me into your ~/.signature to help me spread! -
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 -
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. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum -
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 -
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. -
