Different behaviour between clang ToT and Xcode with __attribute((ns_consumed)) |
|||||||
Issue description
So I reproduced and minimised the linking error I got when building with __attribute((ns_consumed)).
The reason you could not reproduce locally was because the error only happens with ToT clang and not with Xcode clang. To reproduce, you can unzip the attached archive and you'll see that the compilation with ToT clang fails with the following linking error:
Undefined symbols for architecture x86_64:
"i::t::t(objc_object* ns_consumed)", referenced from:
t<NSObject* __strong>::t(NSObject* ns_consumed) in m.o
In fact, we can see that the method has a different signature when built with ARC (m.o) or not (t.o):
$ nm ToT/t.o|c++filt|grep 'i::t::t('
0000000000000020 T i::t::t(objc_object*)
0000000000000000 T i::t::t(objc_object*)
[sdefresne@sdefresne-macbookpro] [~/Developer/upstream/src/repro] (ios-arc)
$ nm ToT/m.o|c++filt|grep 'i::t::t('
U i::t::t(objc_object* ns_consumed)
,
Apr 14 2016
This is still the case with clang version 3.9.0 (trunk 266294).
,
May 4 2016
It looks like the ns_consumed annotation was supposed to be part of the mangled name but was not due to a bug fixed with revision 262278: $ svn log -r 262278 ------------------------------------------------------------------------ r262278 | rjmccall | 2016-03-01 01:49:02 +0100 (Tue, 01 Mar 2016) | 5 lines Generalize the consumed-parameter array on FunctionProtoType to allow arbitrary data to be associated with a parameter. Also, fix a bug where we apparently haven't been serializing this information for the last N years. ------------------------------------------------------------------------ However, the code adding the annotation to the parameter is gated behind a check for LangOpts.ObjCAutoRefCount, which cause the link failure. I think this test is incorrect, and the following patch do fix the link failure: $ svn diff Index: lib/Sema/SemaType.cpp =================================================================== --- lib/Sema/SemaType.cpp (revision 268373) +++ lib/Sema/SemaType.cpp (working copy) @@ -4222,7 +4222,7 @@ } } - if (LangOpts.ObjCAutoRefCount && Param->hasAttr<NSConsumedAttr>()) { + if (Param->hasAttr<NSConsumedAttr>()) { ExtParameterInfos[i] = ExtParameterInfos[i].withIsConsumed(true); HasAnyInterestingExtParameterInfos = true; } With it, the example links on my machine, the code generated does not change (i.e. ns_consumed is still correctly used) and all tests of clang do pass (at least the tests that are run when I use "LLVM_FORCE_HEAD_REVISION=1 tools/clang/scripts/update.py --run-tests" in a Chromium checkout). thakis: how should I go to get this patch in clang repository? Should I create a bug there and propose my CL to fix it? Or can you take care of it?
,
May 10 2016
,
May 10 2016
Sent upstream change set http://reviews.llvm.org/D20113.
,
May 26 2016
Found another problem with ns_consumed. It appears that the codegen is incorrect for constructors (possibly other methods too) were it insert a superfluous objc_release without a corresponding obcj_retain. Reported as https://llvm.org/bugs/show_bug.cgi?id=27887.
,
Dec 20 2016
External dependency fixed. Will need to wait till next version of Xcode (probably during 2017Q3). |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by stkhapugin@chromium.org
, Apr 7 2016