Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

[Lance]

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

Re: Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

[Lukas Fleischer]

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

Re: Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

[Lance]

On 8/26/2013 2:18 PM, Lukas Fleischer wrote:
> 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?
I also had to change line 224 to the following

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

Once both places were changes, it compiled successfully.
thanks
>> 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

Re: Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

[Lukas Fleischer]

On Mon, Aug 26, 2013 at 02:29:12PM -0600, Lance wrote:
> [...]
> >>     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?
> [...]

Re: Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

[Jeff King]

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

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

[PATCH] config: do not use C function names as struct members

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH] config: do not use C function names as struct members

[Lukas Fleischer]

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:
> 
> > 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 <peff@peff.net>
> ---
>  config.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> [...]

Re: [PATCH] config: do not use C function names as struct members

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Thanks.

> -- >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 <peff@peff.net>
> ---
>  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);
>  			}
>  		}
>  	}

Re: Issue with compiling git 1.8.4 under uclibc with gcc 4.6.3

[Andreas Schwab]

Lukas Fleischer <git@cryptocrack.de> writes:

> On Mon, Aug 26, 2013 at 02:29:12PM -0600, Lance wrote:
>> [...]
>> >>     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.

No, it isn't.  fgetc may be a function-like macro that expands to an
arbitrary expression (same for ungetc or ftell, or any other indentifier
that matches a C library function, for that matter).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."