Use llvm.used for __objc_protorefs and __objc_protolist #27346

jinlin-bayarea · 2019-09-24T23:30:10Z

The purpose of the changes is to use llvm.used for __objc_protorefs and __objc_protolist
in the IRGen in Swift compiler to avoid runtime crash.

Resolves SR-11246.

compnerd · 2019-09-25T04:41:35Z

test/IRGen/objc_protocol_vars.sil

@@ -0,0 +1,39 @@
+// RUN: %target-swift-frontend -disable-autolinking-runtime-compatibility-dynamic-replacements -runtime-compatibility-version none -primary-file %s -emit-ir | %FileCheck -check-prefix CHECK %s


You should be able to use -parse-as-library and drop main.

aschwaighofer · 2019-09-25T15:26:38Z

I think this is the right fix. The problem that this PR addresses is if one LTO's swift and objective c objects together. If objective c uses protocol descriptors without adding it to llvm.used it would explain current failures.

AFAICT clang eagerly adds references to protocols when it generates protocols (https://github.com/apple/swift-clang/blob/f667f9fe4ab9da2a82db0291609f2480c30f079c/lib/CodeGen/CGObjCMac.cpp#L6978) and so there would be no need for clang to add them to llvm.used at protocol use sites.

Swift doing the same makes sense to me.

@rjmccall do you agree?

compnerd · 2019-09-25T16:56:04Z

@aschwaighofer yeah, that is correct. This should match the clang behaviour and fix the LTO cases.

jinlin-bayarea · 2019-09-25T17:12:54Z

Thank you for pointing that out. I have fixed the test case and committed it. Jin

…

On Tue, Sep 24, 2019 at 9:43 PM Saleem Abdulrasool ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/IRGen/objc_protocol_vars.sil <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apple_swift_pull_27346-23discussion-5Fr327929343&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=KZqTaO6FWodhuO5iRSr5Nw&m=rpwh5h93FpbkwUC1xrk1D58DRRvofamNaGTAwGN1jAM&s=5ovknABs7gJb0IkHdyNMjO8j3qeOjIeFZu9GIKqOdl8&e=> : > @@ -0,0 +1,39 @@ +// RUN: %target-swift-frontend -disable-autolinking-runtime-compatibility-dynamic-replacements -runtime-compatibility-version none -primary-file %s -emit-ir | %FileCheck -check-prefix CHECK %s You should be able to use -parse-as-library and drop main. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apple_swift_pull_27346-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DALGEZHJAQPP3UBCIRMDMGZLQLLT73A5CNFSM4I2F5HW2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCF2CT2A-23pullrequestreview-2D292825576&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=KZqTaO6FWodhuO5iRSr5Nw&m=rpwh5h93FpbkwUC1xrk1D58DRRvofamNaGTAwGN1jAM&s=o9oEWeV-FEgrVffJenc-sC_j-fszVTlHB_WvzPdpdy8&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ALGEZHIULHYI7R5RB6YNDOTQLLT73ANCNFSM4I2F5HWQ&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=KZqTaO6FWodhuO5iRSr5Nw&m=rpwh5h93FpbkwUC1xrk1D58DRRvofamNaGTAwGN1jAM&s=rodl8AjD6Ndmlc4edPpf-bCmSe3ZTgGfH8jJkMlYFdg&e=> .

rjmccall

LGTM.

rjmccall · 2019-09-25T19:57:57Z

@swift-ci Please test

swift-ci · 2019-09-25T20:00:39Z

Build failed
Swift Test Linux Platform
Git Sha - f151af5a164a2b4232b648532bd6530aeb5310ef

swift-ci · 2019-09-25T20:02:58Z

Build failed
Swift Test OS X Platform
Git Sha - f151af5a164a2b4232b648532bd6530aeb5310ef

rajbarik · 2019-09-26T16:19:29Z

@swift-ci Please test

swift-ci · 2019-09-26T16:21:45Z

Build failed
Swift Test Linux Platform
Git Sha - e7e4fcde3ab1f1a66da456a442eb99ba57f1b37a

swift-ci · 2019-09-26T16:23:15Z

Build failed
Swift Test OS X Platform
Git Sha - e7e4fcde3ab1f1a66da456a442eb99ba57f1b37a

jinlin-bayarea · 2019-09-27T20:27:30Z

@swift-ci Please test

rajbarik · 2019-09-27T21:15:24Z

@swift-ci Please test

swift-ci · 2019-09-27T21:16:00Z

Build failed
Swift Test Linux Platform
Git Sha - 9b4891a8dc25fadbb693c5242724e314c38516a3

rajbarik · 2019-09-28T01:50:07Z

@swift-ci benchmark

rajbarik · 2019-09-28T01:54:20Z

@swift-ci test source compatibility.

rajbarik · 2019-09-28T01:56:12Z

lib/IRGen/GenObjC.cpp

@@ -363,6 +363,10 @@ IRGenModule::getObjCProtocolGlobalVars(ProtocolDecl *proto) {
  protocolLabel->setSection(GetObjCSectionName("__objc_protolist",
                                               "coalesced,no_dead_strip"));

+  // Add __objc_protolist into llvm.used to avoid runtime crash.


@jinlin-bayarea might help to add some information regarding the crash, perhaps LTO?

I think that a comment along the lines of:

Mark used to prevent DCE of public unreferenced protocols to ensure that they are available for external use when used a module is used as a library.

should be good.

swift-ci · 2019-09-28T02:16:39Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
DataAppendArray	2900	3300	+13.8%	0.88x (?)

Improvement	OLD	NEW	DELTA	RATIO
DropLastAnyCollection	13	12	-7.7%	1.08x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
PrefixArray	13	14	+7.7%	0.93x (?)

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

rajbarik · 2019-10-01T18:20:21Z

@swift-ci Please test

rajbarik · 2019-10-01T18:20:57Z

@swift-ci test source compatibility.

rajbarik · 2019-10-01T18:21:20Z

@swift-ci benchmark

swift-ci · 2019-10-01T18:22:24Z

Build failed
Swift Test OS X Platform
Git Sha - 0991b3de9528aa6b487e05f79b9ccafb2941efff

swift-ci · 2019-10-01T19:09:25Z

Performance: -O

Improvement	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStubFromNSStringRef	105	93	-11.4%	1.13x (?)
SuffixAnyCollection	13	12	-7.7%	1.08x (?)

Code size: -O

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
SuffixArray	5	4	-20.0%	1.25x (?)
FlattenListLoop	3136	2686	-14.3%	1.17x (?)
MapReduceShortString	14	13	-7.1%	1.08x (?)

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

swift-ci · 2019-10-01T19:32:46Z

Build failed
Swift Test Linux Platform
Git Sha - c00a7ec799b54d7e96a23129768559dd4fb3bf4b

…dd test to verify.

rajbarik · 2019-10-02T04:30:18Z

@swift-ci benchmark

swift-ci · 2019-10-02T04:51:41Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	2801	3149	+12.4%	0.89x (?)
FlattenListFlatMap	5640	6092	+8.0%	0.93x (?)

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

rajbarik · 2019-10-02T15:44:37Z

@swift-ci Please test

rajbarik · 2019-10-03T04:02:41Z

@swift-ci Smoke test

rajbarik · 2019-10-03T16:42:54Z

@aschwaighofer @compnerd @aschwaighofer @rjmccall This looks good now and I am merging it.

jinlin-bayarea changed the title ~~Llvm used~~ use llvm.used for __objc_protorefs and __objc_protolist Sep 24, 2019

jinlin-bayarea changed the title ~~use llvm.used for __objc_protorefs and __objc_protolist~~ Use llvm.used for __objc_protorefs and __objc_protolist Sep 24, 2019

jrose-apple requested review from rjmccall and aschwaighofer September 24, 2019 23:41

compnerd reviewed Sep 25, 2019

View reviewed changes

rjmccall approved these changes Sep 25, 2019

View reviewed changes

rajbarik reviewed Sep 28, 2019

View reviewed changes

compnerd approved these changes Sep 28, 2019

View reviewed changes

jinlin-bayarea force-pushed the llvm_used branch from 0991b3d to c00a7ec Compare October 1, 2019 18:18

[IRGen] Use llvm.used for __objc_protorefs and __objc_protolist and a…

9e414da

…dd test to verify.

jinlin-bayarea force-pushed the llvm_used branch from c00a7ec to 9e414da Compare October 1, 2019 19:54

rajbarik merged commit 333f0db into swiftlang:master Oct 3, 2019

		@@ -0,0 +1,39 @@
		// RUN: %target-swift-frontend -disable-autolinking-runtime-compatibility-dynamic-replacements -runtime-compatibility-version none -primary-file %s -emit-ir \| %FileCheck -check-prefix CHECK %s

Use llvm.used for __objc_protorefs and __objc_protolist #27346

Use llvm.used for __objc_protorefs and __objc_protolist #27346

Uh oh!

Conversation

jinlin-bayarea commented Sep 24, 2019

Uh oh!

compnerd Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Sep 25, 2019

Uh oh!

jinlin-bayarea commented Sep 25, 2019 via email

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall commented Sep 25, 2019

Uh oh!

swift-ci commented Sep 25, 2019

Uh oh!

swift-ci commented Sep 25, 2019

Uh oh!

rajbarik commented Sep 26, 2019

Uh oh!

swift-ci commented Sep 26, 2019

Uh oh!

swift-ci commented Sep 26, 2019

Uh oh!

jinlin-bayarea commented Sep 27, 2019

Uh oh!

rajbarik commented Sep 27, 2019

Uh oh!

swift-ci commented Sep 27, 2019

Uh oh!

rajbarik commented Sep 28, 2019

Uh oh!

rajbarik commented Sep 28, 2019

Uh oh!

rajbarik Sep 28, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Sep 28, 2019

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Sep 28, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

rajbarik commented Oct 1, 2019

Uh oh!

rajbarik commented Oct 1, 2019

Uh oh!

rajbarik commented Oct 1, 2019

Uh oh!

swift-ci commented Oct 1, 2019

Uh oh!

swift-ci commented Oct 1, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

swift-ci commented Oct 1, 2019

Uh oh!

rajbarik commented Oct 2, 2019

Uh oh!

swift-ci commented Oct 2, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

aschwaighofer commented Sep 25, 2019 •

edited

Loading