Hi Everyone,
The attached patch modifies CGObjCGNU to take the selector type from the front end. This fixes the crash caused by parts of the code expecting a packed structure and parts expecting an unpacked one for the selector type.
I've also included some fixes in this patch which turn NULL into (void*)0 when it is used as a sentinel in variadic functions. This gets rid of a load of warnings when compiling and stops it breaking on 64-bit architectures.
David
clang.diff (9.41 KB)
Why are you getting this? There are tons of other places that pass NULL as a sentinel. Is NULL getting defined incorrectly for that file? NULL needs to be a pointer type, so it should be fine on 64-bit systems. Passing a raw "0" would not be.
-Chris
Chris Lattner wrote:
I've also included some fixes in this patch which turn NULL into
(void*)0 when it is used as a sentinel in variadic functions. This
gets rid of a load of warnings when compiling and stops it breaking
on 64-bit architectures.
Why are you getting this? There are tons of other places that pass
NULL as a sentinel. Is NULL getting defined incorrectly for that
file? NULL needs to be a pointer type, so it should be fine on 64-bit
systems. Passing a raw "0" would not be.
In C++, NULL cannot be a pointer type, because there is no implicit cast
from void* to other pointers. So it is usually defined to be 0 or 0L. In
GCC, it's defined to be __null, but that's just 0 with a warning when
used as an integer.
So some compiler might be smart enough to convert its __null equivalent
to pointer size when passed as vararg, or to define NULL to an integer
constant that is actually the platform pointer size (e.g. 0L for GCC,
but 0LL for MSVC), but I really wouldn't rely on it. The standard fails
to specify that NULL must be as wide as a pointer (a defect IMO), so you
can't trust the constant.
It's one of the most subtle and evil gotchas in C++, really.
Sebastian
nullptr might be one of those C++0X features worth prioritizing.
-Howard
NULL is #defined as 0 for C++ ((void*)0 for C) on FreeBSD. I get missing sentinels in a huge numbers of missing sentinel errors when building LLVM. When I first came across this, I assumed it was a bug in the implementation, but checking the spec apparently it can't be a void* because C++ doesn't allow the type escaping here. In other places I've worked around this by defining NULLP in C++ as a C NULL and using this in sentinels.
On some 64-bit systems it will work fine, since the sentinel will be passed in a register and will be sign extended. This is somewhat dependent on the number of other arguments though - I have a couple of variadic calls here with enough arguments that the sentinel will go on the stack on at least some architectures (SPARC64, for example), at which point it will cause some stack corruption if the stack doesn't happen to have 32 0 bits immediately before the sentinel.
David
Howard Hinnant wrote:
nullptr might be one of those C++0X features worth prioritizing.
Yes. The semantics of nullptr in the face of varargs are correct.
However, prioritizing nullptr in Clang is of little use unless we can
actually compile Clang with Clang, and are willing to accept that it
won't compile on other compilers. I very much doubt this is the case.
Sebastian
NULL is #defined as 0 for C++ on FreeBSD.
This is wrong. If you're using a recent g++ based compiler, NULL should be __null in g++. 
I get missing sentinels in a huge numbers of missing sentinel errors when
building LLVM.
And do they go away if you have NULL defined correctly?
I agree with Mike here. FreeBSD should be using __null or 0L. I don't think we should switch CGObjCGNU to using a different idiom than the rest of the code base, and I don't want to change the entire LLVM world to avoid NULL in varargs.
If FreeBSD headers are broken and they are unwilling to fix it, we can probably do a hack for FreeBSD in the makefiles.
-Chris
I'll try to do something to fix it... thnx for noticing the bug
Chris Lattner wrote:
I agree with Mike here. FreeBSD should be using __null or 0L. I
don't think we should switch CGObjCGNU to using a different idiom than
the rest of the code base, and I don't want to change the entire LLVM
world to avoid NULL in varargs.If FreeBSD headers are broken and they are unwilling to fix it, we can
probably do a hack for FreeBSD in the makefiles.
You'll have to do that for Win64 anyway. From what I can see in VC++
2008 Express, it defines NULL as 0, period.
Do we have anyone testing Win64?
Sebastian
I can't find a copy of the spec online (and I leant my paper copy to someone a while ago), but every reference I can find says that NULL may be a #define for 0 or 0L. Neither of these is guaranteed to be pointer sized or bigger (although 0L is on most platforms, with Win64 being the most notable exception that springs to mind and caused a lot of breakage for code that assumed sizeof(long) >= sizeof(void*)). As I recall, the rationale for NULL in C++ was compatibility with C code, and its use was discourage in new C++ code.
If NULL is #define'd to __null on some platforms, then this is great, but isn't it a GCC extension? I was under the impression that the LLVM coding conventions stated that GCC extensions were to be avoided so that it could be easily portable to compilers.
I don't really care whether this part of the diff is committed - the part I am interested in is the part that makes clang able to compile Objective-C code on non-Apple platforms again - but it would be nice to be able to compile both clang and LLVM without myriad warnings.
David
I can't find a copy of the spec online (and I leant my paper copy to someone a while ago), but every reference I can find says that NULL may be a #define for 0 or 0L.
Fine, but this is irrelevant to the question at hand. The question at hand is an internal implementation detail of a particular compiler, that compiler being g++ on FreeBSD. They apparently goofed. That's a bug in FreeBSD, and it needs to be fixed. Once fixed, there is no issue here.
If NULL is #define'd to __null on some platforms, then this is great, but isn't it a GCC extension?
I'd call it a private internal implementation detail of the compiler. The compiler is implemented such that for proper operation, NULL _must_ be __null.
I was under the impression that the LLVM coding conventions stated that GCC extensions were to be avoided so that it could be easily portable to compilers.
llvm uses NULL. NULL is not a gcc extension.
I can't find a copy of the spec online (and I leant my paper copy to someone a while ago), but every reference I can find says that NULL may be a #define for 0 or 0L.
Fine, but this is irrelevant to the question at hand. The question at hand is an internal implementation detail of a particular compiler, that compiler being g++ on FreeBSD. They apparently goofed. That's a bug in FreeBSD, and it needs to be fixed. Once fixed, there is no issue here.
The question at hand is whether, according to the C++ spec, NULL, when passed to a variadic function, will always be a 0 pointer. I can't find a single reference to support this. If you have one, then it's a bug in g++ combined with the FreeBSD headers. If you don't, then it's a fault in LLVM's usage of NULL, which happens not to matter on some platforms.
If NULL is #define'd to __null on some platforms, then this is great, but isn't it a GCC extension?
I'd call it a private internal implementation detail of the compiler. The compiler is implemented such that for proper operation, NULL _must_ be __null.
No, the compiler is implemented such that, for NULL to be a pointer when passed as an argument to a variadic function it must be __null. Since the C++ spec appears to allow NULL to be 0 or 0L, for compatibility with C code and the C++ type system, this means that passing NULL as a pointer when it is an argument to variadic functions is either a special case in the spec or a GCC extension.
I have never heard of NULL having any special behaviour when passed as an argument to variadic functions (it is a preprocessor macro - the compiler shouldn't need to be aware of it at all after the source is preprocessed). The fact that there is no such behaviour was cited as the main motivation for the introduction of one of the extensions added in C++0x.
I was under the impression that the LLVM coding conventions stated that GCC extensions were to be avoided so that it could be easily portable to compilers.
llvm uses NULL. NULL is not a gcc extension.
Totally irrelevant. Once again, the question is whether NULL is guaranteed to be a type which will be passed as a pointer or a pointer-sized integer to variadic functions, not whether NULL is in the standard.
David
The only description of NULL that I can find is 18.1/4:
"The macro NULL is an implementation-defined C++ null pointer constant
in this International Standard (4.10).[footnote: Possible definitions
include 0 and 0L, but not (void*)0.]"
A "null pointer constant" is, by 4.10, is an integral constant rvalue
expression that evaluates to zero.
Since variable argument don't trigger any implicit conversions, NULL
has no reason to become a pointer.
(Especially since, as you mentioned, the NULL macro is expanded in
translation phase 4, well before the syntactic and semantic analysis
and translation of phase 7.)
~ Scott
#define NULL (3/2-1)
P.S. If you're looking for a legal electronic copy, consider the 1997
public review version ( 1997 C++ Public Review Document
) and the april 2004 draft (
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1638.pdf ), a
combination of which should give a good picture of C++2003.
The question at hand is whether, according to the C++ spec, NULL, when passed to a variadic function, will always be a 0 pointer.
Odd, the issue you seemed to have raised was relating to warnings on freebsd, not some theoretic point about NULL.
Anyway, technically, no. However, dusty deck code requires this of implementations. The standard arguably should just require this. I'm surprised posix didn't just insist, sometimes they do.
If you have one, then it's a bug in g++ combined with the FreeBSD headers.
One doesn't need a reference for this to be a bug in FreeBSD.
If you don't, then it's a fault in LLVM's usage of NULL, which happens not to matter on some platforms.
Arguably, the usage of NULL is theoretically bad:
More C++ Idioms/nullptr - Wikibooks, open books for an open world
But, I'm deeply against causing major pains for 99% of the people that can manage to work on a quality implementation, just because someone can pedantically come up with a broken system that trivially can just be fixed:
#ifdef FreeBSD
#ifdef __GNUG__
#undef NULL
#define NULL __null
#endif
even if we needed to do this on the client side.
However, if you want llvm's sources to be pedantically correct, just turn on -Wstrict-null-sentinel and prest, everyone will be hit by this, not just you. I'm not advocating that however.
If NULL is #define'd to __null on some platforms, then this is great, but isn't it a GCC extension?
I'd call it a private internal implementation detail of the compiler. The compiler is implemented such that for proper operation, NULL _must_ be __null.
No, the compiler is implemented such that, for NULL to be a pointer when passed as an argument to a variadic function it must be __null.
No, the compiler is implemented such that NULL is __null. Honest.
Since the C++ spec appears to allow NULL to be 0 or 0L,
No, it does allow that. Read the standard again. The specs says that g++ gets to define it, and g++ defines it as __null. Since it is defined to be __null, it _cannot_ be any other value, including 0 or 0L.
for compatibility with C code and the C++ type system, this means that passing NULL as a pointer when it is an argument to variadic functions is either a special case in the spec or a GCC extension.
We call it a quality of implementation.
I have never heard of NULL having any special behaviour
Welcome to g++.
The fact that there is no such behaviour was cited as the main motivation for the introduction of one of the extensions added in C++0x.
I'm skeptical that nullptr is the right solution.
I was under the impression that the LLVM coding conventions stated that GCC extensions were to be avoided so that it could be easily portable to compilers.
llvm uses NULL. NULL is not a gcc extension.
Totally irrelevant. Once again, the question is whether NULL is guaranteed to be a type which will be passed as a pointer or a pointer-sized integer to variadic functions, not whether NULL is in the standard.
No, technically you need to do: (char *)NULL.
So, what are you interested in, the theoretics of hard core pedantictry or the engineering realities of having FreeBSD be warning free?
I'm interested in fixing the warning only in as far as the warning indicates potentially unsafe code. The code, as written and according to the language specification, says pass a zero integer value of some indeterminate size to terminate the variadic function. The callee expects a zero pointer value. This is categorically wrong. If some compilers on some architectures do the right thing, then that's a nice bonus. On x86/FreeBSD, the compiler does the right thing (since NULL is a pointer-sized int) but it warns that this is a coincidence.
The #ifdef block you suggest makes the warning go away on FreeBSD/x86, but this is not a platform where the real problem actually occurs, just one which notices that the code is wrong and issues a warning. Warning that the code does not say what you think it does is not a bug, it is a (useful) feature.
The diff I submitted does the right thing, according to the standard. If this is not what you want, then please update the coding conventions to specify this exemption. As it stands, my diff will avoid real issues on platforms like Win64, where 0 and 0L, the two suggested definitions of NULL from the standard, are both smaller than pointers. I have variadic function calls in this file with more pointer arguments than there are integer registers on x86-64 for argument passing, so the 0 will be passed on the stack on this platform and not extended to 64-bits.
Honestly, the amount of effort people have put in to arguing against fixing this one file is less than the amount of effort it would have taken to make the change everywhere in LLVM. I put these changes in with the fixing the type conversion diff because I assumed there would be no debate about fixing incorrect code. It seems I was wrong, so have attached a diff that only fixes the type conversion error, and leaves the unsafe variadic function calls in.
David
clang.diff (1.35 KB)
This patch doesn't apply for me:
patching file lib/CodeGen/CGObjCGNU.cpp
patch: **** malformed patch at line 33: @@ -797,6 +800,9 @@
-Chris
This didn't apply either, but I did it manually:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090119/011121.html
However, why are you using 'getTypeAtIndex'? That always returns i64 for a pointer, it is not returning the first element of the struct. Is that what you want?
-Chris
What _is_ NULL defined to on Win64?