Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

Up until the latest release, I've been able to compile git for a uclibc 
based embedded linux. The toolchain I use is from "entware", which is 
based off of the openwrt toolchain.
https://code.google.com/p/wl500g-repo/

Somewhere between git 1.8.3.4 & 1.8.4 there seems to be some changes 
that breaks compilation for my particular situation.  When cross 
compiling I receive the following error.  I also receive the same error 
when compiling natively on the uclibc based device itself.
Not sure if being uclibc based has anything to do with it, but is 
noteworthy. I am not a c programmer.
Both the cross compiler, and embedded device's version off gcc is 4.6.3.
Previous versions compile fine

     CC config.o
config.c: In function 'get_next_char':
config.c:220:14: error: expected identifier before '(' token
config.c:220:14: error: expected statement before ')' token
config.c:220:14: error: expected statement before ')' token
config.c:224:11: error: expected identifier before '(' token

Lance

On Mon, Aug 26, 2013 at 02:10:43PM -0600, Lance wrote:

Up until the latest release, I've been able to compile git for a
uclibc based embedded linux. The toolchain I use is from "entware",
which is based off of the openwrt toolchain.
https://code.google.com/p/wl500g-repo/

Somewhere between git 1.8.3.4 & 1.8.4 there seems to be some changes
that breaks compilation for my particular situation.  When cross
compiling I receive the following error.  I also receive the same
error when compiling natively on the uclibc based device itself.
Not sure if being uclibc based has anything to do with it, but is
noteworthy. I am not a c programmer.
Both the cross compiler, and embedded device's version off gcc is 4.6.3.
Previous versions compile fine

    CC config.o
config.c: In function 'get_next_char':
config.c:220:14: error: expected identifier before '(' token
config.c:220:14: error: expected statement before ')' token
config.c:220:14: error: expected statement before ')' token
config.c:224:11: error: expected identifier before '(' token

Does changing line 220 of config.c to

    int c = (cf->fgetc)(cf);

fix it?

Lance
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 8/26/2013 2:18 PM, Lukas Fleischer wrote:

On Mon, Aug 26, 2013 at 02:10:43PM -0600, Lance wrote:

quoted

Up until the latest release, I've been able to compile git for a
uclibc based embedded linux. The toolchain I use is from "entware",
which is based off of the openwrt toolchain.
https://code.google.com/p/wl500g-repo/

Somewhere between git 1.8.3.4 & 1.8.4 there seems to be some changes
that breaks compilation for my particular situation.  When cross
compiling I receive the following error.  I also receive the same
error when compiling natively on the uclibc based device itself.
Not sure if being uclibc based has anything to do with it, but is
noteworthy. I am not a c programmer.
Both the cross compiler, and embedded device's version off gcc is 4.6.3.
Previous versions compile fine

     CC config.o
config.c: In function 'get_next_char':
config.c:220:14: error: expected identifier before '(' token
config.c:220:14: error: expected statement before ')' token
config.c:220:14: error: expected statement before ')' token
config.c:224:11: error: expected identifier before '(' token

Does changing line 220 of config.c to

     int c = (cf->fgetc)(cf);

fix it?

I also had to change line 224 to the following

                 c = (cf->fgetc)(cf);

Once both places were changes, it compiled successfully.
thanks

quoted

Lance
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Aug 26, 2013 at 02:29:12PM -0600, Lance wrote:

[...]

quoted

quoted

    CC config.o
config.c: In function 'get_next_char':
config.c:220:14: error: expected identifier before '(' token
config.c:220:14: error: expected statement before ')' token
config.c:220:14: error: expected statement before ')' token
config.c:224:11: error: expected identifier before '(' token

Does changing line 220 of config.c to

    int c = (cf->fgetc)(cf);

fix it?

I also had to change line 224 to the following

                c = (cf->fgetc)(cf);

Once both places were changes, it compiled successfully.

Sounds like a parser bug to me. Should we patch this in Git in order to
make it compile with (broken?) GCC versions?

[...]

On Mon, Aug 26, 2013 at 10:31:54PM +0200, Lukas Fleischer wrote:

quoted

I also had to change line 224 to the following

                c = (cf->fgetc)(cf);

Once both places were changes, it compiled successfully.

Sounds like a parser bug to me. Should we patch this in Git in order to
make it compile with (broken?) GCC versions?

Hmm. I wonder if fgetc is a macro in uclibc? Just grepping their
stdio.h, it looks like that is a possibility.

I think they are probably wrong to do so (but I'd have to check the
standard). However, the cleaner workaround would probably be to call the
fgetc struct member something else.

-Peff

On Mon, Aug 26, 2013 at 04:59:01PM -0400, Jeff King wrote:

Hmm. I wonder if fgetc is a macro in uclibc? Just grepping their
stdio.h, it looks like that is a possibility.

I think they are probably wrong to do so (but I'd have to check the
standard). However, the cleaner workaround would probably be to call the
fgetc struct member something else.

Nope, it's allowed. I think we should do the patch below:

-- >8 --
Subject: config: do not use C function names as struct members

According to C99, section 7.1.4:

  Any function declared in a header may be additionally
  implemented as a function-like macro defined in the
  header.

Therefore calling our struct member function pointer "fgetc"
may run afoul of unwanted macro expansion when we call:

  char c = cf->fgetc(cf);

This turned out to be a problem on uclibc, which defines
fgetc as a macro and causes compilation failure.

The standard suggests fixing this in a few ways:

  1. Using extra parentheses to inhibit the function-like
     macro expansion. E.g., "(cf->fgetc)(cf)". This is
     undesirable as it's ugly, and each call site needs to
     remember to use it (and on systems without the macro,
     forgetting will compile just fine).

  2. Using #undef (because a conforming implementation must
     also be providing fgetc as a function). This is
     undesirable because presumably the implementation was
     using the macro for a performance benefit, and we are
     dropping that optimization.

Instead, we can simply use non-colliding names.

Signed-off-by: Jeff King <redacted>
---
 config.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/config.c b/config.c
index e13a7b6..9f9bf0c 100644
--- a/config.c
+++ b/config.c

@@ -27,9 +27,9 @@ struct config_source {
 	struct strbuf value;
 	struct strbuf var;
 
-	int (*fgetc)(struct config_source *c);
-	int (*ungetc)(int c, struct config_source *conf);
-	long (*ftell)(struct config_source *c);
+	int (*do_fgetc)(struct config_source *c);
+	int (*do_ungetc)(int c, struct config_source *conf);
+	long (*do_ftell)(struct config_source *c);
 };
 
 static struct config_source *cf;

@@ -217,13 +217,13 @@ static int get_next_char(void)
 
 static int get_next_char(void)
 {
-	int c = cf->fgetc(cf);
+	int c = cf->do_fgetc(cf);
 
 	if (c == '\r') {
 		/* DOS like systems */
-		c = cf->fgetc(cf);
+		c = cf->do_fgetc(cf);
 		if (c != '\n') {
-			cf->ungetc(c, cf);
+			cf->do_ungetc(c, cf);
 			c = '\r';
 		}
 	}

@@ -992,9 +992,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 		top.u.file = f;
 		top.name = filename;
 		top.die_on_error = 1;
-		top.fgetc = config_file_fgetc;
-		top.ungetc = config_file_ungetc;
-		top.ftell = config_file_ftell;
+		top.do_fgetc = config_file_fgetc;
+		top.do_ungetc = config_file_ungetc;
+		top.do_ftell = config_file_ftell;
 
 		ret = do_config_from(&top, fn, data);
 

@@ -1013,9 +1013,9 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
 	top.u.buf.pos = 0;
 	top.name = name;
 	top.die_on_error = 0;
-	top.fgetc = config_buf_fgetc;
-	top.ungetc = config_buf_ungetc;
-	top.ftell = config_buf_ftell;
+	top.do_fgetc = config_buf_fgetc;
+	top.do_ungetc = config_buf_ungetc;
+	top.do_ftell = config_buf_ftell;
 
 	return do_config_from(&top, fn, data);
 }

@@ -1196,7 +1196,7 @@ static int store_aux(const char *key, const char *value, void *cb)
 				return 1;
 			}
 
-			store.offset[store.seen] = cf->ftell(cf);
+			store.offset[store.seen] = cf->do_ftell(cf);
 			store.seen++;
 		}
 		break;

@@ -1223,19 +1223,19 @@ static int store_aux(const char *key, const char *value, void *cb)
 		 * Do not increment matches: this is no match, but we
 		 * just made sure we are in the desired section.
 		 */
-		store.offset[store.seen] = cf->ftell(cf);
+		store.offset[store.seen] = cf->do_ftell(cf);
 		/* fallthru */
 	case SECTION_END_SEEN:
 	case START:
 		if (matches(key, value)) {
-			store.offset[store.seen] = cf->ftell(cf);
+			store.offset[store.seen] = cf->do_ftell(cf);
 			store.state = KEY_SEEN;
 			store.seen++;
 		} else {
 			if (strrchr(key, '.') - key == store.baselen &&
 			      !strncmp(key, store.key, store.baselen)) {
 					store.state = SECTION_SEEN;
-					store.offset[store.seen] = cf->ftell(cf);
+					store.offset[store.seen] = cf->do_ftell(cf);
 			}
 		}
 	}

-- 
1.8.4.2.g87d4a77

On Mon, Aug 26, 2013 at 05:57:18PM -0400, Jeff King wrote:

On Mon, Aug 26, 2013 at 04:59:01PM -0400, Jeff King wrote:

quoted

Hmm. I wonder if fgetc is a macro in uclibc? Just grepping their
stdio.h, it looks like that is a possibility.

I think they are probably wrong to do so (but I'd have to check the
standard). However, the cleaner workaround would probably be to call the
fgetc struct member something else.

Nope, it's allowed. I think we should do the patch below:

-- >8 --
Subject: config: do not use C function names as struct members
[...]

Instead, we can simply use non-colliding names.

Yes, this sounds like a better idea to me. So, FWIW, +1 from me.

Signed-off-by: Jeff King <redacted>
---
 config.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

[...]