patchTrivial warning fix for imap-send.c

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>

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

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]

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

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

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:

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

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

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.

Timo Hirvonen <tihirvon@gmail.com> wrote:

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

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

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]

On Sun, 12 Mar 2006 17:32:51 +0000 (UTC) Mark Wooding <mdw@distorted.org.uk> 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.

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

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

In practice (void *)0 is good enough.

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

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

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.

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.

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

On Sun, 12 Mar 2006, Mark Wooding wrote:

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

Linus Torvalds <torvalds@osdl.org> 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.

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]

Mark Wooding wrote:

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.

A Large Angry SCM wrote:

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

H. Peter Anvin wrote:

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.

On Sun, 12 Mar 2006, A Large Angry SCM wrote:

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

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

On Sun, 12 Mar 2006, H. Peter Anvin wrote:

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

		Linus

On Sun, Mar 12, 2006 at 10:37:36PM -0800, H. Peter Anvin wrote:

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.

On Sun, Mar 12, 2006 at 11:20:27AM -0800, A Large Angry SCM 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).

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

Jeff King wrote:

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

On Sun, 12 Mar 2006, Mark Wooding wrote:

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