[PATCH] Trivial warning fix for imap-send.c

[PATCH] Trivial warning fix for imap-send.c

[Art Haas]

Hi.

After my 'git' repo this morning and building I noticed a GCC warning
about a missing sentinel in this file. A scan of the libc docs says
that execl() needs to end with a terminating NULL, as the miniscule
change below does, and recompliation with GCC removed the warning.

Art Haas

Signed-off-by: Art Haas <ahaas@airmail.net>

diff --git a/imap-send.c b/imap-send.c
index fddaac0..203284d 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -945,7 +945,7 @@ imap_open_store( imap_server_conf_t *srv
 				_exit( 127 );
 			close( a[0] );
 			close( a[1] );
-			execl( "/bin/sh", "sh", "-c", srvc->tunnel, 0 );
+			execl( "/bin/sh", "sh", "-c", srvc->tunnel, NULL );
 			_exit( 127 );
 		}
 
-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: [PATCH] Trivial warning fix for imap-send.c

[Mark Wooding]

"Art Haas" <ahaas@airmail.net> wrote:

> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -945,7 +945,7 @@ imap_open_store( imap_server_conf_t *srv
>  				_exit( 127 );
>  			close( a[0] );
>  			close( a[1] );
> -			execl( "/bin/sh", "sh", "-c", srvc->tunnel, 0 );
> +			execl( "/bin/sh", "sh", "-c", srvc->tunnel, NULL );
>  			_exit( 127 );
>  		}

This is not the right fix.  NULL can be simply a #define for 0 (see
6.3.2.3#3 and 7.17).  You need to write (char *)0 or (char *)NULL.  I
prefer to avoid the macro NULL entirely, since its misleading behaviour
is precisely what got us into this mess.

-- [mdw]

Re: [PATCH] Trivial warning fix for imap-send.c

[Junio C Hamano]

Mark Wooding <mdw@distorted.org.uk> writes:

> This is not the right fix.  NULL can be simply a #define for 0 (see
> 6.3.2.3#3 and 7.17).  You need to write (char *)0 or (char *)NULL.  I
> prefer to avoid the macro NULL entirely, since its misleading behaviour
> is precisely what got us into this mess.

Patches welcome.  We have about 15 or so such instances.

$ git grep -n -H 'execl[_a-z]*(' '*.c'
cat-file.c:139:			return execl_git_cmd("ls-tree", argv[2], NULL);
connect.c:547:		execlp(git_proxy_command, git_proxy_command, host, port, NULL);
connect.c:646:			execlp(ssh, ssh_basename, host, command, NULL);
connect.c:654:			execlp("sh", "sh", "-c", command, NULL);
daemon.c:263:	execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", NULL);
exec_cmd.c:97:int execl_git_cmd(const char *cmd,...)
fetch-clone.c:32:		execl_git_cmd("index-pack", "-o", idx, pack_tmp_name, NULL);
fetch-clone.c:109:		execl_git_cmd("unpack-objects", quiet ? "-q" : NULL, NULL);
git.c:256:	execlp("man", "man", page, NULL);
imap-send.c:948:			execl( "/bin/sh", "sh", "-c", srvc->tunnel, NULL );
merge-index.c:18:		execlp(pgm, arguments[0],
pager.c:14:	execlp(prog, prog, NULL);
rsh.c:106:		execlp(ssh, ssh_basename, host, command, NULL);
upload-pack.c:92:	execl_git_cmd("pack-objects", "--stdout", NULL);

[PATCH] Use explicit pointers for execl...() sentinels.

[Mark Wooding]

A terminator of `0' (or `NULL', which might well expand to `0') gets
passed as type `int' in the absence of argument type declarations (which
is the case for execl...(), since it uses varargs).  Argument passing
conventions may differ between `int' and `char *' if, say, `int' is 32
bits and pointers a 64; and there's no particular guarantee that a null
pointer has all-bits-zero anyway.

Signed-off-by: Mark Wooding <mdw@distorted.org.uk>

---

Junio C Hamano <junkio@cox.net> wrote:

> Patches welcome.  We have about 15 or so such instances.

So we do! ;-)  

---

 cat-file.c    |    2 +-
 connect.c     |    6 +++---
 daemon.c      |    2 +-
 fetch-clone.c |    4 ++--
 git.c         |    2 +-
 imap-send.c   |    2 +-
 merge-index.c |    2 +-
 pager.c       |    2 +-
 rsh.c         |    2 +-
 upload-pack.c |    2 +-
 10 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/cat-file.c b/cat-file.c
index 1a613f3..6310787 100644
--- a/cat-file.c
+++ b/cat-file.c
@@ -136,7 +136,7 @@ int main(int argc, char **argv)
 
 		/* custom pretty-print here */
 		if (!strcmp(type, "tree"))
-			return execl_git_cmd("ls-tree", argv[2], NULL);
+			return execl_git_cmd("ls-tree", argv[2], (char*)0);
 
 		buf = read_sha1_file(sha1, type, &size);
 		if (!buf)
diff --git a/connect.c b/connect.c
index 3f2d65c..a86a111 100644
--- a/connect.c
+++ b/connect.c
@@ -544,7 +544,7 @@ static int git_proxy_connect(int fd[2], 
 		close(pipefd[0][1]);
 		close(pipefd[1][0]);
 		close(pipefd[1][1]);
-		execlp(git_proxy_command, git_proxy_command, host, port, NULL);
+		execlp(git_proxy_command, git_proxy_command, host, port, (char *)0);
 		die("exec failed");
 	}
 	fd[0] = pipefd[0][0];
@@ -643,7 +643,7 @@ int git_connect(int fd[2], char *url, co
 				ssh_basename = ssh;
 			else
 				ssh_basename++;
-			execlp(ssh, ssh_basename, host, command, NULL);
+			execlp(ssh, ssh_basename, host, command, (char *)0);
 		}
 		else {
 			unsetenv(ALTERNATE_DB_ENVIRONMENT);
@@ -651,7 +651,7 @@ int git_connect(int fd[2], char *url, co
 			unsetenv(GIT_DIR_ENVIRONMENT);
 			unsetenv(GRAFT_ENVIRONMENT);
 			unsetenv(INDEX_ENVIRONMENT);
-			execlp("sh", "sh", "-c", command, NULL);
+			execlp("sh", "sh", "-c", command, (char *)0);
 		}
 		die("exec failed");
 	}
diff --git a/daemon.c b/daemon.c
index a1ccda3..13d3974 100644
--- a/daemon.c
+++ b/daemon.c
@@ -260,7 +260,7 @@ static int upload(char *dir)
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
 
 	/* git-upload-pack only ever reads stuff, so this is safe */
-	execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", NULL);
+	execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", (char *)0);
 	return -1;
 }
 
diff --git a/fetch-clone.c b/fetch-clone.c
index da1b3ff..14a1cdf 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c
@@ -29,7 +29,7 @@ static int finish_pack(const char *pack_
 		dup2(pipe_fd[1], 1);
 		close(pipe_fd[0]);
 		close(pipe_fd[1]);
-		execl_git_cmd("index-pack", "-o", idx, pack_tmp_name, NULL);
+		execl_git_cmd("index-pack", "-o", idx, pack_tmp_name, (char *)0);
 		error("cannot exec git-index-pack <%s> <%s>",
 		      idx, pack_tmp_name);
 		exit(1);
@@ -106,7 +106,7 @@ int receive_unpack_pack(int fd[2], const
 		dup2(fd[0], 0);
 		close(fd[0]);
 		close(fd[1]);
-		execl_git_cmd("unpack-objects", quiet ? "-q" : NULL, NULL);
+		execl_git_cmd("unpack-objects", quiet ? "-q" : (char *)0, (char *)0);
 		die("git-unpack-objects exec failed");
 	}
 	close(fd[0]);
diff --git a/git.c b/git.c
index 0b40e30..8dd7933 100644
--- a/git.c
+++ b/git.c
@@ -253,7 +253,7 @@ static void show_man_page(const char *gi
 		page = p;
 	}
 
-	execlp("man", "man", page, NULL);
+	execlp("man", "man", page, (char *)0);
 }
 
 static int cmd_version(int argc, const char **argv, char **envp)
diff --git a/imap-send.c b/imap-send.c
index 1b38b3a..825a5cf 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -946,7 +946,7 @@ imap_open_store( imap_server_conf_t *srv
 				_exit( 127 );
 			close( a[0] );
 			close( a[1] );
-			execl( "/bin/sh", "sh", "-c", srvc->tunnel, NULL );
+			execl( "/bin/sh", "sh", "-c", srvc->tunnel, (char *)0);
 			_exit( 127 );
 		}
 
diff --git a/merge-index.c b/merge-index.c
index 024196e..24d4b62 100644
--- a/merge-index.c
+++ b/merge-index.c
@@ -23,7 +23,7 @@ static void run_program(void)
 			    arguments[5],
 			    arguments[6],
 			    arguments[7],
-			    NULL);
+			    (char *)0);
 		die("unable to execute '%s'", pgm);
 	}
 	if (waitpid(pid, &status, 0) < 0 || !WIFEXITED(status) || WEXITSTATUS(status)) {
diff --git a/pager.c b/pager.c
index 1364e15..9da76ac 100644
--- a/pager.c
+++ b/pager.c
@@ -11,7 +11,7 @@ static void run_pager(void)
 	if (!prog)
 		prog = "less";
 	setenv("LESS", "-S", 0);
-	execlp(prog, prog, NULL);
+	execlp(prog, prog, (char *)0);
 }
 
 void setup_pager(void)
diff --git a/rsh.c b/rsh.c
index d665269..92ace2f 100644
--- a/rsh.c
+++ b/rsh.c
@@ -103,7 +103,7 @@ int setup_connection(int *fd_in, int *fd
 		close(sv[1]);
 		dup2(sv[0], 0);
 		dup2(sv[0], 1);
-		execlp(ssh, ssh_basename, host, command, NULL);
+		execlp(ssh, ssh_basename, host, command, (char *)0);
 	}
 	close(sv[0]);
 	*fd_in = sv[1];
diff --git a/upload-pack.c b/upload-pack.c
index 47560c9..fbfdbcd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -89,7 +89,7 @@ static void create_pack_file(void)
 	dup2(fd[0], 0);
 	close(fd[0]);
 	close(fd[1]);
-	execl_git_cmd("pack-objects", "--stdout", NULL);
+	execl_git_cmd("pack-objects", "--stdout", (char *)0);
 	die("git-upload-pack: unable to exec git-pack-objects");
 }
 

-- [mdw]

Re: [PATCH] Use explicit pointers for execl...() sentinels.

[Timo Hirvonen]

On Sun, 12 Mar 2006 13:59:42 +0000 (UTC)
Mark Wooding <mdw@distorted.org.uk> wrote:

> A terminator of `0' (or `NULL', which might well expand to `0') gets
> passed as type `int' in the absence of argument type declarations (which
> is the case for execl...(), since it uses varargs).  Argument passing
> conventions may differ between `int' and `char *' if, say, `int' is 32
> bits and pointers a 64; and there's no particular guarantee that a null
> pointer has all-bits-zero anyway.

NULL should always be ((void *)0).  What 64-bit systems declare NULL as
plain 0 (not 0L)?  How about fixing those systems instead of making the
git source code unreadable.

-- 
http://onion.dynserv.net/~timo/

Re: [PATCH] Use explicit pointers for execl...() sentinels.

[Mark Wooding]

Timo Hirvonen <tihirvon@gmail.com> wrote:

> NULL should always be ((void *)0).

Says who?  I already gave chapter and verse for what NULL is required to
do.

Besides, (void *)0 fixes /this particular/ problem, because `void *' and
`char *' have the same representation (6.2.5#27).  This wouldn't help us
with a putative function which takes an arbitrary number of `foo *'
pointers, since nothing guarantees that `void *' and `foo *' have
similar representations.  You'd have to say `(foo *)0' or `(foo *)NULL'.

> What 64-bit systems declare NULL as plain 0 (not 0L)?

Don't know: didn't look.  0L won't do the right thing with IL32LLP64, if
anyone was actually crazy enough to specify such an ABI.  The point is,
there's not much 

> How about fixing those systems instead of making the git source code
> unreadable.

Because, according to the C and POSIX specs, they're not wrong.

The right fix from the point of view of a C implementation would be to
define NULL to be some weird __null_pointer token which the compiler
could warn about whenever it was used in an untyped argument context.

(Besides, I don't find bare or casted `0' unreadable.  Maybe I'm just
strange.)

-- [mdw]

Re: [PATCH] Use explicit pointers for execl...() sentinels.

[Timo Hirvonen]

On Sun, 12 Mar 2006 17:32:51 +0000 (UTC)
Mark Wooding <mdw@distorted.org.uk> wrote:

> Besides, (void *)0 fixes /this particular/ problem, because `void *' and
> `char *' have the same representation (6.2.5#27).  This wouldn't help us
> with a putative function which takes an arbitrary number of `foo *'
> pointers, since nothing guarantees that `void *' and `foo *' have
> similar representations.  You'd have to say `(foo *)0' or `(foo *)NULL'.

NULL pointer does not point to any data, it just says it's 'empty'.  So
it doesn't need to be same type pointer as specified in the function
prototype.  Pointers are just addresses, it doesn't matter from to code
generation point of view whether it is (char *)0 or (void *)0.

> Don't know: didn't look.  0L won't do the right thing with IL32LLP64, if
> anyone was actually crazy enough to specify such an ABI.  The point is,
> there's not much 

sizeof(unsigned long) is sizeof(void *) in real world.

> > How about fixing those systems instead of making the git source code
> > unreadable.
> 
> Because, according to the C and POSIX specs, they're not wrong.

They didn't think of 64-bit architectures back then, I suppose.

> The right fix from the point of view of a C implementation would be to
> define NULL to be some weird __null_pointer token which the compiler
> could warn about whenever it was used in an untyped argument context.

In practice (void *)0 is good enough.

> (Besides, I don't find bare or casted `0' unreadable.  Maybe I'm just
> strange.)

'ugly' would have been better word than 'unreadable'.

-- 
http://onion.dynserv.net/~timo/

Re: [PATCH] Use explicit pointers for execl...() sentinels.

[Jeff King]

On Sun, Mar 12, 2006 at 08:08:12PM +0200, Timo Hirvonen wrote:

> NULL pointer does not point to any data, it just says it's 'empty'.  So
> it doesn't need to be same type pointer as specified in the function
> prototype.  Pointers are just addresses, it doesn't matter from to code
> generation point of view whether it is (char *)0 or (void *)0.

Sorry, but I think you're wrong according to the C standard. Pointers of 
different types do NOT have to share the same representation (e.g.,
there have been some platforms where char* and int* were different
sizes). A void pointer must be capable of representing any type of
pointer (for example, holding the largest possible type). However, if
sizeof(void *) == 8 and sizeof(char *) == 4, you have a problem with
variadic functions which are expecting to pull 4 byte off the stack. 

In a non-variadic function, the compiler would do the right implicit
casting. In a variadic function, it can't. 

The real question is, does git want to care about portability to such
platforms.

If you remain unconvinced, I can try to find chapter and verse of the
standard.

> sizeof(unsigned long) is sizeof(void *) in real world.

Are you saying that because it encompasses all of the platforms you've
worked on, or do you have some evidence that it is largely the case? It
certainly isn't guaranteed by the C standard.

> > Because, according to the C and POSIX specs, they're not wrong.
> They didn't think of 64-bit architectures back then, I suppose.

No, they did think of those issues; they intentionally left such sizing
up to the implementation to allow C to grow with the hardware. Mostly
you don't have to care, but as I said, typing with variadic functions is
a pain.

-Peff

Re: [PATCH] Use explicit pointers for execl...() sentinels.

[Horst von Brand]

Jeff King <peff@peff.net> wrote:
> On Sun, Mar 12, 2006 at 08:08:12PM +0200, Timo Hirvonen wrote:
> > NULL pointer does not point to any data, it just says it's 'empty'.  So
> > it doesn't need to be same type pointer as specified in the function
> > prototype.  Pointers are just addresses, it doesn't matter from to code
> > generation point of view whether it is (char *)0 or (void *)0.

> Sorry, but I think you're wrong according to the C standard. Pointers of 
> different types do NOT have to share the same representation (e.g.,
> there have been some platforms where char* and int* were different
> sizes). A void pointer must be capable of representing any type of
> pointer (for example, holding the largest possible type). However, if
> sizeof(void *) == 8 and sizeof(char *) == 4,

Very improbable, they'll be the same normally ("void *" is a way of getting
rid of the overloading of the meaning of "char *" for this before ANSI C).
Sure, sizeof(int *) might be 4, but I think that is pretty far off.

>                                              you have a problem with
> variadic functions which are expecting to pull 4 byte off the stack. 

There are special rules for variadic functions, probably pointers would be
cast to/from void * in such a case by the compiler.

> In a non-variadic function, the compiler would do the right implicit
> casting. In a variadic function, it can't. 

It sure can. The rules where defined so that it works.

> The real question is, does git want to care about portability to such
> platforms.

Broken platform, on which the compiler fails miserably in doing its job?
No, it doesn't.

> If you remain unconvinced, I can try to find chapter and verse of the
> standard.

Please do.

> > sizeof(unsigned long) is sizeof(void *) in real world.

> Are you saying that because it encompasses all of the platforms you've
> worked on, or do you have some evidence that it is largely the case? It
> certainly isn't guaranteed by the C standard.

More because a machine with pointers that are much larger than the largest
"normal" integer would be pretty weird (sure, on intel 8086 they where 32
("far" pointer, segment + offset) and 16 bits, but...
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

[OT] Re: [PATCH] Use explicit pointers for execl...() sentinels.

[Jeff King]

On Mon, Mar 13, 2006 at 12:12:31AM -0400, Horst von Brand wrote:

> Very improbable, they'll be the same normally ("void *" is a way of getting
> rid of the overloading of the meaning of "char *" for this before ANSI C).
> Sure, sizeof(int *) might be 4, but I think that is pretty far off.

Let me clarify my position. The STANDARD doesn't guarantee such things.
In PRACTICE, for modern machines you can assume that all pointers are
the same size (and things like all-bits-zero is a null pointer) if it
makes your code cleaner. In other words, I agree with Linus: git should
follow what works in practice, but you should at least recognize that
you're violating the standard.

That being said, you appear to be making the argument that passing a
'foo *' to a variadic function expecting a 'bar *' doesn't violate the
standard. I believe it invokes undefined behavior.

> There are special rules for variadic functions, probably pointers would be
> cast to/from void * in such a case by the compiler.

The rules indicate that arguments matching the '...' follow "default
argument promotion".  See section 6.5.2.2, paragraph 7.  This default
promotion is the same as what would happen if there were no prototype
for the function, and is defined in paragraph 6:
  ...the integer promotions are performed on each argument, and arguments
  that have type float are promoted to double.
I don't see anything about promoting pointers to void.

Furthermore, when accessing the arguments using va_arg, the types must
match or the behavior is undefined, UNLESS (7.5.1.1, para 2):
  - one type is signed and the other is the matching unsigned type
  - one type is a pointer to void and the other is a pointer of
    character type
IOW, the standard does promise that void* and char* pointers are
represented the same, but nothing else.

> > If you remain unconvinced, I can try to find chapter and verse of the
> > standard.
> Please do.

See above.

-Peff

Re: [PATCH] Trivial warning fix for imap-send.c

[Linus Torvalds]

On Sun, 12 Mar 2006, Mark Wooding wrote:
> "Art Haas" <ahaas@airmail.net> wrote:
> 
> > -			execl( "/bin/sh", "sh", "-c", srvc->tunnel, 0 );
> > +			execl( "/bin/sh", "sh", "-c", srvc->tunnel, NULL );
> 
> This is not the right fix.  NULL can be simply a #define for 0 (see
> 6.3.2.3#3 and 7.17).  You need to write (char *)0 or (char *)NULL.  I
> prefer to avoid the macro NULL entirely, since its misleading behaviour
> is precisely what got us into this mess.

It's perfectly fine.

Quite frankly, if you have a 64-bit C compiler that doesn't make "NULL" be 
"((void *)0)" (or equivalent - some compilers will actually have NULL as 
an intrisic, because especially if they also support C++, NULL has some 
really magical properties there), you should switch vendors as quickly as 
humanly possible.

The "#define NULL 0" practice is still _legal_ C, but that doesn't make it 
any less broken. It's K&R traditional, but git requires ANSI prototypes 
and some fancy features from modern compilers, so K&R compilers aren't 
welcome anyway, and if their headers don't define NULL as a void pointer, 
their headers are simply _broken_.

So in modern C, using NULL at the end of a varargs array as a pointer is 
perfectly sane, and the extra cast is just ugly and bowing to bad 
programming practices and makes no sense to anybody who never saw the 
horror that is K&R.

It's akin to trying to not using prototypes, or to trying to limit your 
externally visible names to 7 characters. It was "appropriate" about two 
decades ago, these days it's just cuddling broken setups that have been 
broken for a long long time.

Btw, the reason NULL _has_ to be a pointer ("((void *)0)" or otherwise) is 
simply that if it isn't, you not only won't get reasonable varargs 
behaviour, you'll also miss real warnings. I've seen broken code like

	int i = NULL;

in my life, and if the compiler doesn't warn about that, then the compiler 
is BROKEN, and not worth supporting as a programmer.

			Linus

Re: [PATCH] Trivial warning fix for imap-send.c

[Mark Wooding]

Linus Torvalds <torvalds@osdl.org> wrote:

> So in modern C, using NULL at the end of a varargs array as a pointer is 
> perfectly sane, and the extra cast is just ugly and bowing to bad 
> programming practices and makes no sense to anybody who never saw the 
> horror that is K&R.

No!  You can still get bitten.  You're lucky that on common platforms
all pointers look the same, but if you find one where `char *' (and
hence `void *') isn't the same as `struct foo *' then, under appropriate
circumstances you /will/ unless you put the casts in.

Now, maybe we don't care for GIT.  That's your (and Junio's) call.  My
natural approach is to work as closely as I can to the specs (and then
throw in hacks for platforms which /still/ don't work), though, which is
why I brought the subject up.

-- [mdw]

Re: [PATCH] Trivial warning fix for imap-send.c

[A Large Angry SCM]

Mark Wooding wrote:
> Linus Torvalds <torvalds@osdl.org> wrote:
> 
>>So in modern C, using NULL at the end of a varargs array as a pointer is 
>>perfectly sane, and the extra cast is just ugly and bowing to bad 
>>programming practices and makes no sense to anybody who never saw the 
>>horror that is K&R.
> 
> No!  You can still get bitten.  You're lucky that on common platforms
> all pointers look the same, but if you find one where `char *' (and
> hence `void *') isn't the same as `struct foo *' then, under appropriate
> circumstances you /will/ unless you put the casts in.

Please explain how malloc() can work on such a platform. My reading of 
the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ 
cast-able to/from a void * and that NULL should be #defined as (void *). 
See 3.2.2.3 and 4.1.5 if interested.

Re: [PATCH] Trivial warning fix for imap-send.c

[H. Peter Anvin]

A Large Angry SCM wrote:
> Mark Wooding wrote:
> 
>> Linus Torvalds <torvalds@osdl.org> wrote:
>>
>>> So in modern C, using NULL at the end of a varargs array as a pointer 
>>> is perfectly sane, and the extra cast is just ugly and bowing to bad 
>>> programming practices and makes no sense to anybody who never saw the 
>>> horror that is K&R.
>>
>> No!  You can still get bitten.  You're lucky that on common platforms
>> all pointers look the same, but if you find one where `char *' (and
>> hence `void *') isn't the same as `struct foo *' then, under appropriate
>> circumstances you /will/ unless you put the casts in.
> 
> Please explain how malloc() can work on such a platform. My reading of 
> the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ 
> cast-able to/from a void * and that NULL should be #defined as (void *). 
> See 3.2.2.3 and 4.1.5 if interested.

Consider the non-hypothetical example of a word-addressed machine, which 
has to have extra bits in a subword pointer like char *.  The C standard 
requires that void * has those bits as well, but it doesn't means that 
any void * can be cast to any arbitrary pointer -- the opposite, 
however, is required.

	-hpa

Re: [PATCH] Trivial warning fix for imap-send.c

[A Large Angry SCM]

H. Peter Anvin wrote:
> A Large Angry SCM wrote:
>> Mark Wooding wrote:
>>
>>> Linus Torvalds <torvalds@osdl.org> wrote:
>>>
>>>> So in modern C, using NULL at the end of a varargs array as a 
>>>> pointer is perfectly sane, and the extra cast is just ugly and 
>>>> bowing to bad programming practices and makes no sense to anybody 
>>>> who never saw the horror that is K&R.
>>>
>>> No!  You can still get bitten.  You're lucky that on common platforms
>>> all pointers look the same, but if you find one where `char *' (and
>>> hence `void *') isn't the same as `struct foo *' then, under appropriate
>>> circumstances you /will/ unless you put the casts in.
>>
>> Please explain how malloc() can work on such a platform. My reading of 
>> the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ 
>> cast-able to/from a void * and that NULL should be #defined as (void 
>> *). See 3.2.2.3 and 4.1.5 if interested.
> 
> Consider the non-hypothetical example of a word-addressed machine, which 
> has to have extra bits in a subword pointer like char *.  The C standard 
> requires that void * has those bits as well, but it doesn't means that 
> any void * can be cast to any arbitrary pointer -- the opposite, 
> however, is required.

ANSI X3.159-1989

3.2.2.3 Pointers
	A pointer to *void* may be converted to or from a pointer to any 
incomplete or object type. A pointer to any incomplete or object type 
may be converted to a pointer to *void* and back again; the result shall 
compare equal to the original pointer.

For any qualifier /q/, a pointer to a non-/q/-qualified type may be 
converted to a pointer to the /q/-qualified version of the type; the 
values stored in the original and converted pointers shall compare equal.

In integral constant expression with value 0, or such an expression cast 
to type <bold>void *</bold>, is called a /null pointer constant.[*33*] 
If a null pointer constant is assigned to or compared for equality to a 
pointer, the constant is converted to a pointer of that type. Such a 
pointer, called a /null pointer/, is guaranteed to compare unequal to a 
pointer to any object or function.

Two null pointers, converted through possibly different sequences of 
casts to pointer types, shall compare equal.

[*33*] the macro *NULL* is defined in <stddef.h> as a null pointer 
constant; see 4.1.5.

Re: [PATCH] Trivial warning fix for imap-send.c

[Linus Torvalds]

On Sun, 12 Mar 2006, A Large Angry SCM wrote:
> 
> 3.2.2.3 Pointers
> 	A pointer to *void* may be converted to or from a pointer to any
> incomplete or object type. A pointer to any incomplete or object type may be
> converted to a pointer to *void* and back again; the result shall compare
> equal to the original pointer.

Large, you're missing the point.

"void *" is guaranteed to be a _superset_ of all pointers.

But that dos not mean that any "void *" pointer can be cast to any other 
pointer. BUT IF IT STARTED OUT AS A POINTER OF THAT TYPE, IT'S GUARANTEED 
THAT IT CAN BE CAST _BACK_ TO THAT TYPE.

(And furthermore, NULL is special in that it will always compare equal 
regardless of how it has ever been cast).

This means, for example, that it's perfectly legal for a C implementation 
to have a 128-bit "void *", where the low bits are the "real pointer" and 
the high 64 bits are the "type descriptor". You could only cast such a 
pointer to that proper type, but you could not cast it to any other type 
(except for the special case of NULL).

My argument boils down to the fact that we don't care one whit about those 
theoretical architectures. It so happens that ia64 function pointers are 
sometimes described this way (due to totally broken reasons - don't ask), 
and that function pointers could indeed be seen as 128-bit quantities. But 
that is such a horribly broken thing, that what compilers on ia64 actually 
do is to instead of having a 128-bit "void *" (which would be legal per 
the standard), they make function pointers actually point to the function 
description (128-bit datum) rather than the actual start of the function.

(I think. I forget the exact details. I think the whole architecture is a 
total mess, and should never have been done in the firstplace).

Similarly, there are certain tagged architectures where the pointer 
actually contains the type it points to, and again, C _allows_ that, and 
if you want to be strictly conforming, you can't do certain things that 
seem obviously correct.

HOWEVER. The undeniable fact is that no sane architecture that anybody 
cares about today (and that, in turn, implies that nobody will care about 
it in the next quarter century - these things have a tendency to 
re-inforce themselves) actually does that. 

Another example is two's complement. C as a language actually allows other 
type representations than two's complement for integers, and there's lots 
of verbiage in the standard about how overflow is undefined etc. Then they 
go to pains to explain how "unsigned" integers are guaranteed to behave as 
if the machine was a regular binary machine, even though the language 
lawyers in general went to great pain to make it clear that if the integer 
representation is binary-packed-decimal, it's still legal from a C 
stanpoint.

But again, nobody sane would ever care. The likelihood that we'll see a 
ternary machine in the next few decades is pretty damn small, because 
while the C standard allows for something else, it would be painful in the 
extreme for anybody to actually convert all the programs that effectively 
depend on 8-bit bytes etc.

So again, in _theory_ the C standard works for some really odd crap out 
there. In practice, there are only certain pretty standard setups (ILP32, 
I32LP64, IL32P64), and some old ones (I16LP32) that nobody cares about, 
and then the really odd ones (36-bit word-addressable monsters where char, 
short, int, long and pointer are all the same size) that have a C 
compiler, but that you will never be able to port _any_ normal program 
to..

In other words, the C standard allows some really strange stuff. Trying to 
even worry about it is just not worth it. It often makes the code just 
much harder to read for absolutely zero gain.

So in practice, the strangest setup you'll ever really care about is 
actually Windows. And it's strange because it can have a totally broken 
size model (IL32LLP64 - although I think that's usually just a compiler 
switch), and because it has such strange system libraries and filesystem 
behaviour (which is sadly more than just a compiler switch).

Even windows (or, perhaps, Windows _in_particular_) will never have things 
like a "char" that isn't 8 bits, etc that could be possible in theory if 
you were to just read the C standard.

		Linus

Re: [PATCH] Trivial warning fix for imap-send.c

[H. Peter Anvin]

On "real" machines, the biggest reason you'd care is that a lot of 
compilers, *especially* in C++ mode, really still define NULL as "0"; 
ostensibly because defining it as "((void *)0)" breaks some obscure C++ 
casting rule.

	-hpa

Re: [PATCH] Trivial warning fix for imap-send.c

[Linus Torvalds]

On Sun, 12 Mar 2006, H. Peter Anvin wrote:
>
> On "real" machines, the biggest reason you'd care is that a lot of compilers,
> *especially* in C++ mode, really still define NULL as "0"; ostensibly because
> defining it as "((void *)0)" breaks some obscure C++ casting rule.

Agreed. gcc has fixed that rule, but others have not. Don't compile git 
as C++.

		Linus

Re: [PATCH] Trivial warning fix for imap-send.c

[Olivier Galibert]

On Sun, Mar 12, 2006 at 10:37:36PM -0800, H. Peter Anvin wrote:
> On "real" machines, the biggest reason you'd care is that a lot of 
> compilers, *especially* in C++ mode, really still define NULL as "0"; 
> ostensibly because defining it as "((void *)0)" breaks some obscure C++ 
> casting rule.

Not obscure, just a religious issue.  Somehow in the creation of the
C++ standard the definition of void * got changed from "generic
pointer" to something else I've been unable to fathom.  That
definition, whatever it is, justifies forbidding implicit casts from
void * to anything else.  Some of the priests of the new definition
consider the existence in C of a usable generic pointer type to be a
failing of the language too.

  OG.

Re: [PATCH] Trivial warning fix for imap-send.c

[Jeff King]

On Sun, Mar 12, 2006 at 11:20:27AM -0800, A Large Angry SCM wrote:

> >No!  You can still get bitten.  You're lucky that on common platforms
> >all pointers look the same, but if you find one where `char *' (and
> >hence `void *') isn't the same as `struct foo *' then, under appropriate
> >circumstances you /will/ unless you put the casts in.
> 
> Please explain how malloc() can work on such a platform. My reading of 
> the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ 
> cast-able to/from a void * and that NULL should be #defined as (void *). 
> See 3.2.2.3 and 4.1.5 if interested.

I think Linus has cut to the heart of the discussion (that it's worth
git maintainers' sanity not to worry about such problems). However, for
pedantry's sake, this is how malloc works:

A void pointer is guaranteed to be able to hold any type of pointer
(either char * or struct foo * or whatever). The declaration of malloc
indicates a return of void *. On a platform where it matters, the
compiler generates code so that 
  struct foo *bar = malloc(100);
converts the void * pointer into the correct size (in the same way that
assigning between differently sized integers works).

This breaks down with variadic functions, which have no typing
information. So doing this:
  execl("foo", "bar", my_struct_foo);
doesn't give the compiler a chance to do the implicit cast and you get
subtle breakage (in the same way that you would if you passed a long to
a variadic function expecting a short).

-Peff

Re: [PATCH] Trivial warning fix for imap-send.c

[Horst von Brand]

Jeff King <peff@peff.net> wrote:

[...]

> A void pointer is guaranteed to be able to hold any type of pointer
> (either char * or struct foo * or whatever). The declaration of malloc
> indicates a return of void *. On a platform where it matters, the
> compiler generates code so that 
>   struct foo *bar = malloc(100);
> converts the void * pointer into the correct size (in the same way that
> assigning between differently sized integers works).

Right.

> This breaks down with variadic functions, which have no typing
> information. So doing this:
>   execl("foo", "bar", my_struct_foo);
> doesn't give the compiler a chance to do the implicit cast and you get
> subtle breakage (in the same way that you would if you passed a long to
> a variadic function expecting a short).

It just passes 3 "void *"'s, and casts back. What is so strange?
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] Trivial warning fix for imap-send.c

[Linus Torvalds]

On Mon, 13 Mar 2006, Horst von Brand wrote:
>> 
> > This breaks down with variadic functions, which have no typing
> > information. So doing this:
> >   execl("foo", "bar", my_struct_foo);
> > doesn't give the compiler a chance to do the implicit cast and you get
> > subtle breakage (in the same way that you would if you passed a long to
> > a variadic function expecting a short).
> 
> It just passes 3 "void *"'s, and casts back. What is so strange?

It doesn't actually pass 3 "void *".

Variadic functions pass their arguments as-is (apart from the normal 
integer promotion for small integer types: chars are passed as integers if 
"char" is smaller than "int").

So no actual casting takes place for the arguments, and

	execl("foo", "bar", my_struct_foo, NULL);

passes in four pointers: two of type "char *", one of whatever 
my_struct_foo is ("struct foo *") and finally hopefully one of "void *" 
(modulo broken compilers). They might - in theory - have different sizes 
and representations.

Now, the interesting effect of this is that simple things like

	printf("Pointer value %p\n", myfunction);

is actually NOT STRICTLY PORTABLE CODE! Why? Because "%p" wants a void 
pointer, but "myfunction" is of a different pointer type, which may 
actually have a different size and a different representation entirely, so 
you can get total garbage printed out.

So you have two choices:

 - be sane, and ignore insane architectures. In this case, the above is 
   perfectly fine C code.

 - be insane, and care about it. In this case, you really do have to add 
   the casts to be safe in theory.

The "%p" for printf() is actually a wonderful example of why you really 
really really should ignore language lawyers. According to language 
lawyers, you should add that "(void *)" cast. But look around for how many 
such casts you can find in real code, and realize that the language 
lawyers just don't matter. A C compiler environment that requires it is 
simply broken, and sane people will refuse to use it for anything than 
small embedded work, because it's simply not usable.

So while it's not true in theory, in _practice_ you should expect all 
pointers to be of the same size and use the same representation. Anything 
else is just too painful to be ever worth bothering with.

(Remember near and far pointers from 16-bit DOS/Windows? Those 
environments at least had _explicit_ pointer sizes, making the problem 
less horrible, but that was clearly a disaster nonetheless. Pointers that 
implicitly have different sizes because they point to different types are 
even _worse_).

			Linus

PS. Final words: while knowing that all pointer representations must be 
the same, and that NULL actually must be the bitwise binary "all zero" 
value (otherwise "memset(structptr, 0, structsize)" would break), it's 
equally important to know that the standard doesn't _guarantee_ it. Why? 

Because that way, when somebody tries to tell you that your code isn't 
standards conformant and might be unportable, you can say "yeah, I know, 
but only a total loser would ever care". Instead of incorrectly trying to 
argue that your code is "correct".

Re: [PATCH] Trivial warning fix for imap-send.c

[H. Peter Anvin]

Jeff King wrote:
> 
> I think Linus has cut to the heart of the discussion (that it's worth
> git maintainers' sanity not to worry about such problems). However, for
> pedantry's sake, this is how malloc works:
> 
> A void pointer is guaranteed to be able to hold any type of pointer
> (either char * or struct foo * or whatever). The declaration of malloc
> indicates a return of void *. On a platform where it matters, the
> compiler generates code so that 
>   struct foo *bar = malloc(100);
> converts the void * pointer into the correct size (in the same way that
> assigning between differently sized integers works).
> 

Furthermore, the return value of malloc() (calloc, realloc, ...) is 
guaranteed to be a value suitable for casting to any pointer type for 
which at least one object can fit in the allocated memory block.

	-hpa

Re: [PATCH] Trivial warning fix for imap-send.c

[Horst von Brand]

Mark Wooding <mdw@distorted.org.uk> wrote:
> Linus Torvalds <torvalds@osdl.org> wrote:
> > So in modern C, using NULL at the end of a varargs array as a pointer is 
> > perfectly sane, and the extra cast is just ugly and bowing to bad 
> > programming practices and makes no sense to anybody who never saw the 
> > horror that is K&R.

> No!  You can still get bitten.

Only if the compiler is completely broken.

>                                 You're lucky that on common platforms
> all pointers look the same, but if you find one where `char *' (and
> hence `void *') isn't the same as `struct foo *' then, under appropriate
> circumstances you /will/ unless you put the casts in.

Show one platform where this is true...

> Now, maybe we don't care for GIT.  That's your (and Junio's) call.  My
> natural approach is to work as closely as I can to the specs (and then
> throw in hacks for platforms which /still/ don't work), though, which is
> why I brought the subject up.

If on the platform you use it doesn't work, change the compiler. Fast.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] Trivial warning fix for imap-send.c

[Linus Torvalds]

On Sun, 12 Mar 2006, Mark Wooding wrote:

> Linus Torvalds <torvalds@osdl.org> wrote:
> 
> > So in modern C, using NULL at the end of a varargs array as a pointer is 
> > perfectly sane, and the extra cast is just ugly and bowing to bad 
> > programming practices and makes no sense to anybody who never saw the 
> > horror that is K&R.
> 
> No!  You can still get bitten.  You're lucky that on common platforms
> all pointers look the same, but if you find one where `char *' (and
> hence `void *') isn't the same as `struct foo *' then, under appropriate
> circumstances you /will/ unless you put the casts in.

Not relevant. Show me any system that matters.

The fact is, compilers should conform to programmers, not the other way 
around. Bending over backwards for broken systems is _wrong_. The fact 
that there are insane build environments doesn't excuse bad manners, and 
explicit casts that aren't needed are HORRIBLE manners.

There is no valid reason to _ever_ cast NULL pointers. 

Btw, the same goes for casting the result from malloc etc, which some 
people also do. 

Put another way: you should not encourage insane systems, and you should 
definitely NOT encourage nit-picking people who read the standards in 
insane ways and say that the standards _allow_ badly behaved build 
environments.

It's true that the standards _allow_ crazy build environments. Who the 
f*ck cares? Crazy and bad build environments aren't any better for being 
allowed by the standard. Screw them. Call them names. And refuse to work 
with them.

		Linus