ToTiOS failing compile |
||||||||||
Issue description
FAILED: obj/ios/third_party/earl_grey/earl_grey_arch_shared_library_sources/GREYCAAnimationDelegate.o
../../third_party/llvm-build/Release+Asserts/bin/clang -MMD -MF obj/ios/third_party/earl_grey/earl_grey_arch_shared_library_sources/GREYCAAnimationDelegate.o.d -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_XCODE_VERSION=1010 -DCR_CLANG_REVISION=\"349894\" -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DNS_BLOCK_ASSERTIONS=1 -I../.. -Igen -Igen/ios/third_party/earl_grey/earl_grey/EarlGrey.headers.hmap -I../../ios/third_party/earl_grey/src/EarlGrey -I../../ios/third_party/fishhook/src -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -arch x86_64 -Oz -fno-omit-frame-pointer -gdwarf-2 -isysroot /b/c/xcode_ios_10b61.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator12.1.sdk -stdlib=libc++ -mios-simulator-version-min=11.0 -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang enforce-in-thirdparty-webkit -Xclang -plugin-arg-find-bad-constructs -Xclang check-enum-max-value -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Werror -Wall -Wno-unused-variable -Wunguarded-availability -Wundeclared-selector -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-null-pointer-arithmetic -Wno-ignored-pragma-optimize -fvisibility=default -F . -F . -F /b/c/xcode_ios_10b61.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/Library/Frameworks -std=c11 -fobjc-arc -c ../../ios/third_party/earl_grey/src/EarlGrey/Delegate/GREYCAAnimationDelegate.m -o obj/ios/third_party/earl_grey/earl_grey_arch_shared_library_sources/GREYCAAnimationDelegate.o
../../ios/third_party/earl_grey/src/EarlGrey/Delegate/GREYCAAnimationDelegate.m:87:33: error: 'init' is unavailable
outDelegate = [[self alloc] init];
^
../../ios/third_party/earl_grey/src/EarlGrey/Delegate/GREYCAAnimationDelegate.h:37:1: note: 'init' has been explicitly marked unavailable here
- (instancetype)init NS_UNAVAILABLE;
^
1 error generated.
Since the bot was red for another reason too, the regression window is pretty large: 349509:349894
But `svn log -r349509:349894 https://nico@llvm.org/svn/llvm-project/cfe/trunk/lib/Sema` doesn't contain that much stuff, and my money is on:
------------------------------------------------------------------------
r349841 | arphaman | 2018-12-20 17:11:11 -0500 (Thu, 20 Dec 2018) | 21 lines
[ObjC] Messages to 'self' in class methods that return 'instancetype' should
use the pointer to the class as the result type of the message
Prior to this commit, messages to self in class methods were treated as instance
methods to a Class value. When these methods returned instancetype the compiler
only saw id through the instancetype, and not the Interface *. This caused
problems when that return value was a receiver in a message send, as the
compiler couldn't select the right method declaration and had to rely on a
selection from the global method pool.
This commit modifies the semantics of such message sends and uses class messages
that are dispatched to the interface that corresponds to the class that contains
the class method. This ensures that instancetypes are correctly interpreted by
the compiler. This change is safe under ARC (as self can't be reassigned),
however, it also applies to MRR code as we are assuming that the user isn't
doing anything unreasonable.
rdar://20940997
Differential Revision: https://reviews.llvm.org/D36790
------------------------------------------------------------------------
Need to find out if this was an intentional behavior change, and then either wait for a fix upstream, or change our code.
,
Dec 21
After reading a bit more about NS_UNAVAILABLE, I think this is a bug in earl grey. If that init is marked unavailable, we shouldn't call it. Does that sound right? If so, can we get this fixed upstream and roll that in? This is blocking compiler updates.
,
Dec 21
Assigning to Sheriff for investigation.
,
Dec 21
,
Dec 21
Over to lindsayw for EG stuff.
,
Dec 21
,
Jan 3
This is still a problem and is by now getting kind of urgent. This blocks compiler rolls.
,
Jan 3
This was fixed upstream but hasn't yet been rolled into Chromium. https://chromium-review.googlesource.com/c/chromium/src/+/1394390
,
Jan 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/218268afb8122e0cab4dccf4c0d38c4bbbe40f46 commit 218268afb8122e0cab4dccf4c0d38c4bbbe40f46 Author: Rohit Rao <rohitrao@chromium.org> Date: Thu Jan 03 17:17:48 2019 [ios] Rolls EarlGrey1 to 25fc49c5e57b8d25e4011efdd6a96024d3e6b96e. Fixes a call to an NS_UNAVAILABLE inititalizer. BUG= 917351 Change-Id: I3c1471f3fe0764c8e03d026b28bf7b90e44fe9f5 Reviewed-on: https://chromium-review.googlesource.com/c/1394390 Commit-Queue: Rohit Rao <rohitrao@chromium.org> Commit-Queue: Justin Cohen <justincohen@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Cr-Commit-Position: refs/heads/master@{#619665} [modify] https://crrev.com/218268afb8122e0cab4dccf4c0d38c4bbbe40f46/DEPS [modify] https://crrev.com/218268afb8122e0cab4dccf4c0d38c4bbbe40f46/ios/third_party/earl_grey/BUILD.gn
,
Jan 9
Looks like https://reviews.llvm.org/D56469 will allow this pattern, so once that goes in we don't need to change internal code and could undo the earl_grey change too (but that means we can't land https://chromium-review.googlesource.com/c/chromium/src/+/1400767 and I'll have to wait for that to land and re-build packages).
,
Jan 11
It seems that we rolled clang to a point where the pattern is not available. This leads to a conflict with piper code and we cannot compile at the moment (so we cannot roll chromium). Do we plan to include https://reviews.llvm.org/D56469 in the near future?
,
Jan 11
I rolled to http://reviews.llvm.org/rL350768 which is the revision https://reviews.llvm.org/D56469 landed at. Can you post the error you're seeing?
,
Jan 11
(i.e. I don't think upstream clang is super likely to get more changes related to this, you'll need to clean up that code as well.)
,
Jan 11
,
Jan 11
Summary from the discussion on internal issue 920950:
This diags:
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat test.mm
@interface NSObject
+ (instancetype)alloc;
@end
@interface Sub : NSObject
- (instancetype)init __attribute__((unavailable));
@end
@implementation Sub
+ (Sub*)create {
return [[self alloc] init];
}
@end
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang -c test.mm
test.mm:11:24: error: 'init' is unavailable
return [[self alloc] init];
^
test.mm:6:1: note: 'init' has been explicitly marked unavailable here
- (instancetype)init __attribute__((unavailable));
^
1 error generated.
While this doesn't:
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat test.mm
@interface NSObject
+ (instancetype)alloc;
@end
@interface Sub : NSObject
- (instancetype)init __attribute__((unavailable));
@end
@implementation Sub
+ (Sub*)create {
return [[self alloc] init];
}
- (instancetype)init {
return self;
}
@end
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang -c test.mm
Note the presence of an init function definition in second snippet.
Some internal code had a class deriving from UIButton that marked init as unavailable, but then called init from a factory method -- but without having an implementation of init. So the diagnostic still fires there. I'll raise this with the Apple folks, but it's possible that nothing will change.
,
Jan 11
Raised here: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190107/257267.html Also making this bug public, looks like nothing internal is really mentioned here -- that happened on issue 920950.
,
Jan 15
I think this is no longer an issue. Back to thakis@ to confirm.
,
Jan 15
olivierrobin fixed all the internal code that had this issue. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by thakis@chromium.org
, Dec 21