Deterministically order class members for emission #32581

slavapestov · 2020-06-27T03:16:43Z

We already did this for vtable entries, because its required for correctness; sink the ordering down into EmittedMembersRequest in service of reproducible builds as well.

This fixes an issue found by @davidungar where the driver's batching could flip the order of a class's default init() and deinit members, which tripped up our incremental build testing.

Along the way, I discovered that caching the emitted members list broke deriving the Differentiable conformance, because we might cache the member list before synthesizing the members; fixing this by forcing the conformance in EmittedMembersRequest in turn uncovered some order dependencies in conformance derivation, which I already addressed in #32578.

slavapestov · 2020-06-27T03:25:47Z

lib/Sema/TypeCheckDecl.cpp

@@ -2451,16 +2503,40 @@ EmittedMembersRequest::evaluate(Evaluator &evaluator,
  forceConformance(Context.getProtocol(KnownProtocolKind::Decodable));
  forceConformance(Context.getProtocol(KnownProtocolKind::Encodable));
  forceConformance(Context.getProtocol(KnownProtocolKind::Hashable));
+  forceConformance(Context.getProtocol(KnownProtocolKind::Differentiable));


@dan-zheng @rxwei I think this should be uncontroversial. We need to force the derived members, if any, to be synthesized here, because the result of this request is now cached.

Looks good to me.

davidungar

LGTM! You'll see I have teeny nit, but it's probably not worth the time to address it.

davidungar · 2020-06-27T03:55:01Z

lib/AST/Decl.cpp

@@ -4318,6 +4318,10 @@ GetDestructorRequest::evaluate(Evaluator &evaluator, ClassDecl *CD) const {
  if (ctx.LangOpts.EnableObjCInterop)
    CD->recordObjCMethod(DD, DD->getObjCSelector());

+  // Mark it as synthesized to make its location in getEmittedMembers()
+  // deterministic.


Nice "why" comment!

davidungar · 2020-06-27T03:56:34Z

lib/Sema/TypeCheckDecl.cpp

+    if (auto *cd = dyn_cast<ConstructorDecl>(afd))
+      mangledName = mangler.mangleConstructorEntity(cd, /*allocator=*/false);
+    else if (auto *dd = dyn_cast<DestructorDecl>(afd))
+      mangledName = mangler.mangleDestructorEntity(dd, /*deallocating=*/false);
+    else
+      mangledName = mangler.mangleEntity(afd);


Too bad we don't just have a member function on AbstractFuncDecl to compute this "mangledNameForSorting". I wonder if this functionality is duplicated elsewhere?

mangleEntity() will assert if you pass in a destructor or constructor. The reason we have separate entry points for those is that usually they have multiple manglings, for the (de)allocating and (de)initializing entry points, respectively. I don't think it's worth factoring this out.

slavapestov · 2020-06-29T22:22:06Z

@swift-ci Please test

slavapestov · 2020-06-29T22:22:12Z

@swift-ci Please test source compatibility

swift-ci · 2020-06-29T22:42:57Z

Build failed
Swift Test OS X Platform
Git Sha - 141dfd0e81e42fdea10295c3dedfc438759f0784

slavapestov · 2020-06-29T22:57:59Z

@swift-ci Please test

slavapestov · 2020-06-29T22:58:32Z

@swift-ci Please test source compatibility

swift-ci · 2020-06-30T00:03:48Z

Build failed
Swift Test Linux Platform
Git Sha - 141dfd0e81e42fdea10295c3dedfc438759f0784

swift-ci · 2020-06-30T00:06:02Z

Build failed
Swift Test Linux Platform
Git Sha - 84ec4117157d13d67e0f5d276662e68ef5001886

swift-ci · 2020-06-30T00:10:55Z

Build failed
Swift Test OS X Platform
Git Sha - 84ec4117157d13d67e0f5d276662e68ef5001886

We sometimes need to mangle invalid identifiers, for example when sorting declarations in EmittedMembersRequest. Make sure this doesn't assert.

…rsRequest

Previously we did this in the SILVTableVisitor, since maintaining a consistent order between translation units is important for correctness. However, we also want the order in which the members themselves are emitted to remain consistent regardless of the order in which members were synthesized. This is in service of reproducible builds.

… function order We can synthesize the default init() and the implicit deinit in a class in any order, depending on the order of primary files and how that class was used in other primary files. Make sure that EmittedMembersRequest puts the destructor at the end with all other synthesized members, so that we produce the same object file in any case.

slavapestov · 2020-06-30T02:20:01Z

@swift-ci Please smoke test

Mimic changes in upstream commit: faa06bf Context: #32581 (comment)

compnerd · 2020-07-04T16:49:18Z

I think that this change might be the one responsible for a recent regression with @main usage (CC: @natecook1000):

// RUN: %target_swiftc -parse-as-library -parse-stdlib -module-name Swift -S -o - %s
@main
class S {
  public static func main() {
  }
}

The -parse-as-library is required for @main to be usable, -parse-stdlib -module-name Swift is purely to avoid the dependency on the standard library (trying to create a testcase for the future), You will note that there is a call emitted to $ss1SC5$mainyyFZ (on Linux x86_64, it will be callq ($ss1SC5$mainyyFZ)@PLT and on Windows x86_64 it will be callq ($ss1SC5$mainyyFZ)). This method is not being synthesized any longer, resulting in undefined references.

lxbndr · 2020-07-04T17:21:06Z

I have another sample code which not builds:

public class SomeClass {
}
extension SomeClass: Encodable {	
}

Linker tells about unresolved externals:

srx0-0f6207.o : error LNK2019: unresolved external symbol $s4srx09SomeClassC10CodingKeys33_0A88DACEEBAF8928C0B22F7967630AAFLLOs0D3KeyAAMc referenced in function $s4srx09SomeClassC10CodingKeys33_0A88DACEEBAF8928C0B22F7967630AAFLLOAFs0D3KeyAAWl
srx0.dll : fatal error LNK1120: 1 unresolved externals

Noticed on Windows, but I guess it is multiplatform. I am pretty sure it is caused by this change. Previous commit does not fail.

slavapestov · 2020-07-07T22:30:22Z

@compnerd @lxbndr Thanks for the bug report, I'll take a look.

slavapestov requested review from CodaFi and davidungar June 27, 2020 03:16

slavapestov commented Jun 27, 2020

View reviewed changes

davidungar approved these changes Jun 27, 2020

View reviewed changes

CodaFi approved these changes Jun 27, 2020

View reviewed changes

ASTMangler: Tiny cleanup

ff2cc35

slavapestov force-pushed the cached-emitted-members-request branch from c023ce5 to 141dfd0 Compare June 29, 2020 22:18

slavapestov force-pushed the cached-emitted-members-request branch from 141dfd0 to 84ec411 Compare June 29, 2020 22:57

slavapestov added 5 commits June 29, 2020 22:17

ASTMangler: Fix assertion when mangling invalid identifiers

31a7859

We sometimes need to mangle invalid identifiers, for example when sorting declarations in EmittedMembersRequest. Make sure this doesn't assert.

Sema: Force synthesized conformance to Differentiable in EmittedMembe…

f8720d5

…rsRequest

Sema: Use request evaluator caching for EmittedMembersRequest

c1b0c93

slavapestov force-pushed the cached-emitted-members-request branch from 84ec411 to 89b2f27 Compare June 30, 2020 02:18

slavapestov merged commit faa06bf into swiftlang:master Jun 30, 2020

dan-zheng added a commit that referenced this pull request Jun 30, 2020

[TF] Fix EuclideanDifferentiable derived conformances.

e41c6e6

Mimic changes in upstream commit: faa06bf Context: #32581 (comment)

This was referenced Jul 8, 2020

Request-ify synthesis of the main function for the @main attribute #32767

Merged

Sema: Force synthesis of the CodingKeys nested type in EmittedMembersRequest #32824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deterministically order class members for emission #32581

Deterministically order class members for emission #32581

slavapestov commented Jun 27, 2020 •

edited

Loading

slavapestov Jun 27, 2020

rxwei Jun 27, 2020

davidungar left a comment

davidungar Jun 27, 2020

davidungar Jun 27, 2020

slavapestov Jun 29, 2020

slavapestov commented Jun 29, 2020

slavapestov commented Jun 29, 2020

swift-ci commented Jun 29, 2020

slavapestov commented Jun 29, 2020

slavapestov commented Jun 29, 2020

swift-ci commented Jun 30, 2020

swift-ci commented Jun 30, 2020

swift-ci commented Jun 30, 2020

slavapestov commented Jun 30, 2020

compnerd commented Jul 4, 2020

lxbndr commented Jul 4, 2020

slavapestov commented Jul 7, 2020

Deterministically order class members for emission #32581

Deterministically order class members for emission #32581

Conversation

slavapestov commented Jun 27, 2020 • edited Loading

slavapestov Jun 27, 2020

Choose a reason for hiding this comment

rxwei Jun 27, 2020

Choose a reason for hiding this comment

davidungar left a comment

Choose a reason for hiding this comment

davidungar Jun 27, 2020

Choose a reason for hiding this comment

davidungar Jun 27, 2020

Choose a reason for hiding this comment

slavapestov Jun 29, 2020

Choose a reason for hiding this comment

slavapestov commented Jun 29, 2020

slavapestov commented Jun 29, 2020

swift-ci commented Jun 29, 2020

slavapestov commented Jun 29, 2020

slavapestov commented Jun 29, 2020

swift-ci commented Jun 30, 2020

swift-ci commented Jun 30, 2020

swift-ci commented Jun 30, 2020

slavapestov commented Jun 30, 2020

compnerd commented Jul 4, 2020

lxbndr commented Jul 4, 2020

slavapestov commented Jul 7, 2020

slavapestov commented Jun 27, 2020 •

edited

Loading