Hi,
The attached diff removes the MakeConstantString() method from CGObjCGNU and replaces uses to it with the equivalent method in CodeGenModule. It also updates the non-fragile ABI implementation.
David
clang.diff (11.9 KB)
Updated version of the diff tidies up a few more things, and adds support for the -fconstant-string-class= flag (GCC-compatibility).
David
clang.diff (16.8 KB)
Did you want this in your patch?
Index: test/CodeGenObjC/attr-strong.c
Few comments:
Did you want this in your patch?
No, this happened when I ran the test suite; there appears to be a bug in a pipe somewhere, because the test ran fine the first time then failed the second time because the file was full of nonsense. I thought I'd reverted it (I had, but apparently after I created the diff).
Few comments:
+
+ ObjCConstantStringClass = "NXConstantString";This is specific to Gnu runtime and initialization should happen for when in Gnu-specific runtime mode.
I've made this change for now, but it breaks layering because the default to NXConstantString is for GCC compatibility (should be NSConstantString on the NeXT runtime, NSCFString for the NeXT runtime when -fconstant-cfstrings is provided or the target is Darwin) and so should be set in the driver, not in CodeGen.
-OPTION("-pthread", pthread, Flag, INVALID, INVALID, "", 0, 0, 0)
+OPTION("-pthread", pthread, Flag, INVALID, INVALID, "q", 0, 0, 0)Unrelated to this patch. Please provide a separate patch for this (and a test case) if you want it.
Reverted; this was a quick hack from an earlier diff, I think someone else fixed this in the correct way.
@@ -818,6 +824,9 @@
else if (GNURuntime)
Options.NeXTRuntime = 0;+ if (!ObjCConstantStringClass.empty())
+ Options.ObjCConstantStringClass = ObjCConstantStringClass.c_str();
+This should happen when in GNURuntime. For other runtimes please issue a warning that option is not supported.
This should also be supported for the NeXT runtime - this option was created by NeXT for NeXTSTEP compatibility in OPENSTEP, when the constant string class was renamed from NXConstantString (NeXT namespace, used on NeXTSTEP) to NSConstantString (NeXT-Sun namespace used on OpenStep implementations including OPENSTEP). From the Apple GCC docs:
-fconstant-string-class=class-name
Use class-name as the name of the class to instantiate for each literal string specified with the syntax @"...". The default class name is NXConstantString if the GNU runtime is being used, and NSConstantString if the NeXT runtime is being used (see below). The -fconstant-cfstrings option, if also present, will override the -fconstant-string-class setting and cause @"..." literals to be laid out as constant CoreFoundation strings.
There are other subtle details in the semantics of the NeXT-compatible implementation of this, which I will leave to you.
Please provide a test case for this patch.
I've modified CodeGenObjC/constant-strings.m to test that this works.
Any other comments?
David
Did you want this in your patch?
No, this happened when I ran the test suite; there appears to be a bug in a pipe somewhere, because the test ran fine the first time then failed the second time because the file was full of nonsense. I thought I'd reverted it (I had, but apparently after I created the diff).
Few comments:
+
+ ObjCConstantStringClass = "NXConstantString";This is specific to Gnu runtime and initialization should happen for when in Gnu-specific runtime mode.
I've made this change for now, but it breaks layering because the default to NXConstantString is for GCC compatibility (should be NSConstantString on the NeXT runtime, NSCFString for the NeXT runtime when -fconstant-cfstrings is provided or the target is Darwin) and so should be set in the driver, not in CodeGen.
Currently, there is no plan to support legacy variations of objc's constant strings for NeXT runtime in clang (no -fno-constant-cfstrings or -fconstant-string-class=class-name). Please add the FIXME with above comment if you don't have plan to change the above.
-OPTION("-pthread", pthread, Flag, INVALID, INVALID, "", 0, 0, 0)
+OPTION("-pthread", pthread, Flag, INVALID, INVALID, "q", 0, 0, 0)Unrelated to this patch. Please provide a separate patch for this (and a test case) if you want it.
Reverted; this was a quick hack from an earlier diff, I think someone else fixed this in the correct way.
@@ -818,6 +824,9 @@
else if (GNURuntime)
Options.NeXTRuntime = 0;+ if (!ObjCConstantStringClass.empty())
+ Options.ObjCConstantStringClass = ObjCConstantStringClass.c_str();
+This should happen when in GNURuntime. For other runtimes please issue a warning that option is not supported.
This should also be supported for the NeXT runtime - this option was created by NeXT for NeXTSTEP compatibility in OPENSTEP, when the constant string class was renamed from NXConstantString (NeXT namespace, used on NeXTSTEP) to NSConstantString (NeXT-Sun namespace used on OpenStep implementations including OPENSTEP). From the Apple GCC docs:
As long as there is no support for legacy stuff in Non-Gnu runtime, please issue a warning for those runtimes.
-fconstant-string-class=class-name
Use class-name as the name of the class to instantiate for each literal string specified with the syntax @"...". The default class name is NXConstantString if the GNU runtime is being used, and NSConstantString if the NeXT runtime is being used (see below). The -fconstant-cfstrings option, if also present, will override the -fconstant-string-class setting and cause @"..." literals to be laid out as constant CoreFoundation strings.There are other subtle details in the semantics of the NeXT-compatible implementation of this, which I will leave to you.
Please provide a test case for this patch.
I've modified CodeGenObjC/constant-strings.m to test that this works.
Thanks.
- Fariborz
I've just tested on Snow Leopard, and it seems that the GCC documentation is wrong; the current behaviour is to accept this option but simply discard it. I've committed it without the warning for now, because that mimics gcc behaviour, but I'll add one if you definitely want it.
David
Isn't the -fno-constant-cfstrings flag required to build unloadable bundle ? Is there some change that made it legacy ?
From the Unloading Bundle section:
"When you compile a bundle with a minimum deployment target of Mac OS X 10.2 (or later), the compiler automatically switches to generating strings that are truly constant in response toCFSTR("..."). The compiler also generates these constant strings if you compile with the flag -fconstant-cfstrings. Constant strings have many benefits and should be used when possible, however if you reference constant strings after the executable containing them is unloaded, the references will be invalid and will cause a crash. This might happen even if the strings have been retained, for example, as a result of being put in data structures, retained directly, and, in some cases, even copied. Rather than trying to make sure all such references are cleaned up at unload time (and some references might be created within the libraries, making them hard to track), it is best to compile unloadable bundles with the flag -fno-constant-cfstrings."
This should also be supported for the NeXT runtime - this option was created by NeXT for NeXTSTEP compatibility in OPENSTEP, when the constant string class was renamed from NXConstantString (NeXT namespace, used on NeXTSTEP) to NSConstantString (NeXT-Sun namespace used on OpenStep implementations including OPENSTEP). From the Apple GCC docs:
As long as there is no support for legacy stuff in Non-Gnu runtime, please issue a warning for those runtimes.
I've just tested on Snow Leopard, and it seems that the GCC documentation is wrong; the current behaviour is to accept this option but simply discard it. I've committed it without the warning for now, because that mimics gcc behaviour, but I'll add one if you definitely want it.
To have the effect on SL you need to provide both -fno-constant-cfstrings -fconstant-string-class=xx
It is probably a gcc bug that it silently ignores -fconstant-string-class=xx all by itself. Warning will indeed
be useful to us.
- Thanks, Fariborz
Thanks for pointing this out. I will pose this question to our internal folks.
- Fariborz
To have the effect on SL you need to provide both -fno-constant-
cfstrings -fconstant-string-class=xx
It is probably a gcc bug that it silently ignores -fconstant-string-
class=xx all by itself.
In that case it is a gcc bug. A test program that just prints the class of a constant string produces an NSCFString when compiled with -fconstant-string-class=NSConstantString on SL:
$ cat str.m
#include <stdio.h>
#include <objc/runtime.h>
int main(void)
{
printf("%s\n", class_getName(((id)@"foo")->isa));
return 0;
}
$ gcc -fconstant-string-class=NSConstantString str.m -lobjc
Undefined symbols:
"___CFConstantStringClassReference", referenced from:
_main in cc7mMxrY.o
ld: symbol(s) not found
collect2: ld returned 1 exit status
$ gcc --version
i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5646)
Copyright (C) 2007 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
This should be emitting an error that the NSConstantString class is not found. If you link this against Cocoa, you get NSCFString as the output.
Warning will indeed
be useful to us.
Looking in clang-cc.cpp, it seems that other cases where incompatible options are specified are logging an error with fprintf and exiting. What is the correct way of issuing an option-ignored warning in clang-cc? Or should this be handled in clang?
David
Thanks for pointing this out. I will pose this question to our
internal folks.
The response I got is that we do not need to support -fconstant-string-class (so, warning is warranted).
We should support -fno-constant-cfstrings for Bundle unloading at some point.
- Fariborz