New issue
Advanced search Search tips

Issue 917351 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug

Blocking:
issue 917419
issue 920950



Sign in to add a comment

ToTiOS failing compile

Project Member Reported by thakis@chromium.org, Dec 21

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.
 
Cc: linds...@chromium.org justincohen@chromium.org
+ios folks who might know if the current earl grey code is supposed to look like it does, and to maybe find out why that NS_UNAVAILABLE is there.
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.
Cc: olivierrobin@chromium.org
Owner: michaeldo@chromium.org
Assigning to Sheriff for investigation.
Cc: rohitrao@chromium.org
Status: Assigned (was: Untriaged)
Labels: -Pri-3 Pri-1
Owner: linds...@chromium.org
Over to lindsayw for EG stuff.
Blocking: 917419
This is still a problem and is by now getting kind of urgent. This blocks compiler rolls.
Owner: rohitrao@chromium.org
Status: Started (was: Assigned)
This was fixed upstream but hasn't yet been rolled into Chromium.

https://chromium-review.googlesource.com/c/chromium/src/+/1394390
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Comment 10 Deleted

Comment 11 Deleted

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).
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?
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?
(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.)
Blocking: 920950
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.
Labels: -Restrict-View-Google
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.
Owner: thakis@chromium.org
I think this is no longer an issue. Back to thakis@ to confirm.
Owner: olivierrobin@chromium.org
Status: Fixed (was: Assigned)
olivierrobin fixed all the internal code that had this issue.

Sign in to add a comment