Add a -public-linkage-for-metadata-accessors IRGen flag for LLDB. #23465

adrian-prantl · 2019-03-21T17:49:40Z

In LLDB expressions, references to private metadata accessors may be
emitted and need to be bound to symbols available in the attached
program, even if these symbols are only supposed to have private
visibility within the program.

Also rdar://problem/48018240

adrian-prantl · 2019-03-21T17:50:05Z

@swift-ci test

slavapestov

This feels a bit arbitrary. What about all the other accessor functions IRGen emits, or even just private methods? Maybe instead do a post-processing pass on the LLVM module to make everything public? I also thought at some point the lldb expression evaluator walked to AST making everything public - is that no longer happening?

I’m also worried that “plumbing” boolean parameters like this through multiple layers of calls will lead to a proliferation of boolean conditions that will become hard to reason about as these sort of flags accumulate. I’d be happy to discuss other ways of solving the underlying problem.

rjmccall · 2019-03-21T21:48:00Z

This feels a bit arbitrary. What about all the other accessor functions IRGen emits, or even just private methods? Maybe instead do a post-processing pass on the LLVM module to make everything public?

Or have IRGen's linkage computation understand that private things from the execution context aren't actually object-file-private when running in the debugger. We do something similar with private declarations when building for LTO.

swift-ci · 2019-03-21T23:34:04Z

Build failed
Swift Test Linux Platform
Git Sha - 95c96e83abd11aa36d1cc9d86247ca189b3f0bdd

adrian-prantl · 2019-03-22T00:54:07Z

This feels a bit arbitrary. What about all the other accessor functions IRGen emits, or even just private methods? Maybe instead do a post-processing pass on the LLVM module to make everything public?

Or have IRGen's linkage computation understand that private things from the execution context aren't actually object-file-private when running in the debugger. We do something similar with private declarations when building for LTO.

Can you elaborate on this a bit? So, still have an IRGen flag that specifies that this is for the expression evaluator, but have it apply to more kinds of symbols? What's a reasonable boundary?

A quick grep for LTO in IRGen didn't find any results, what should I be looking for?

rjmccall · 2019-03-22T04:18:11Z

Sorry, I meant for WMO. See getIRLinkage in GenDecl.cpp.

adrian-prantl · 2019-03-26T23:50:35Z

Sorry for the delay.

@rjmccall Is this closer to what you had in mind? The option to communicate that we are in the LLDB expression evaluator is still there, but it is now localized to getIRLinkage().

adrian-prantl · 2019-03-26T23:53:00Z

lib/IRGen/GenDecl.cpp

@@ -1462,6 +1462,9 @@ getIRLinkage(const UniversalLinkageInfo &info, SILLinkage linkage,
    return RESULT(External, Hidden, Default);

  case SILLinkage::Private: {
+    if (info.forcePublicDecls())
+      return getIRLinkage(info, SILLinkage::SharedExternal, isDefinition,
+                          isWeakImported);


I'm also not sure whether this is the best possible set of flags. It appears to work...

It probably ought to be based on whether this is a definition, so that definitions are shared (or maybe even private) and non-definitions are public-external.

slavapestov · 2019-03-27T01:55:48Z

@adrian-prantl LLDB defines a method named SwiftASTManipulator::MakeDeclarationsPublic(). Is it not getting called anymore? Or its not doing the right thing?

rjmccall

Yeah, that's more what I was thinking, thanks.

rjmccall · 2019-03-27T02:31:23Z

include/swift/IRGen/Linking.h

@@ -49,12 +49,15 @@ class UniversalLinkageInfo {
  /// True iff are multiple llvm modules.
  bool HasMultipleIGMs;

+  /// Used by the LLDB expression evaluator.
+  bool ForcePublicDecls;


The comment here should explain what this means, not how it's used.

rjmccall · 2019-03-27T02:33:10Z

lib/IRGen/GenDecl.cpp

@@ -1462,6 +1462,9 @@ getIRLinkage(const UniversalLinkageInfo &info, SILLinkage linkage,
    return RESULT(External, Hidden, Default);

  case SILLinkage::Private: {
+    if (info.forcePublicDecls())
+      return getIRLinkage(info, SILLinkage::SharedExternal, isDefinition,
+                          isWeakImported);


It probably ought to be based on whether this is a definition, so that definitions are shared (or maybe even private) and non-definitions are public-external.

adrian-prantl · 2019-03-27T02:41:09Z

@adrian-prantl LLDB defines a method named SwiftASTManipulator::MakeDeclarationsPublic(). Is it not getting called anymore? Or its not doing the right thing?

IIUC the problem I'm trying to solve here is that Swift metadata accessors particularly are transparent to the AST and only injected/created by IRGen.

slavapestov · 2019-03-27T04:50:47Z

@adrian-prantl The linkage of a metadata accessor is computed from the access level of the nominal type. So if the type is public the accessor should be public too. Is this not happening in some cases?

adrian-prantl · 2019-03-27T15:32:35Z

@adrian-prantl The linkage of a metadata accessor is computed from the access level of the nominal type. So if the type is public the accessor should be public too. Is this not happening in some cases?

The type in the example was private.

For context, the expression is printing a global variable

private struct FixedContainer {
  var s = S() // S is a resilient struct and defined in a different module
}
private var g_main_c = FixedContainer()

In LLDB expressions, references to private metadata accessors may be emitted and need to be bound to symbols available in the attached program, even if these symbols are only supposed to have private visibility within the program. Also rdar://problem/48018240

adrian-prantl · 2019-03-27T16:11:32Z

@swift-ci test

adrian-prantl · 2019-03-27T16:12:35Z

@swift-ci test

slavapestov · 2019-03-27T18:42:10Z

The type in the example was private.

Right, but doesn't SwiftASTManipulator::MakeDeclarationsPublic() make everything public? That's the part I'm confused about.

adrian-prantl · 2019-03-27T19:13:34Z

As far as I understand the type in question is not part of the AST walked by SwiftASTManipulator::Publicist. It walks the expression source file, but our type is part of the swiftmodule that is being debugged.

adrian-prantl · 2019-03-27T19:14:37Z

Note that just because I merged this, doesn't mean we shouldn't still replace this with something better if we find it, but this is blocking all the resilience support.

slavapestov · 2019-03-27T22:21:03Z

@adrian-prantl No worries.

If the Decl is part of the swiftmodule being debugged, why is lldb emitting the type metadata accessor?

jrose-apple · 2019-03-27T22:23:29Z

It's emitting a reference to the existing accessor (a declaration rather than a definition).

slavapestov · 2019-03-27T23:07:22Z

@jrose-apple I see. Perhaps it makes more sense to elevate private linkage to internal when emitting code with -g?

adrian-prantl requested review from jrose-apple and rjmccall March 21, 2019 17:50

slavapestov reviewed Mar 21, 2019

View reviewed changes

adrian-prantl mentioned this pull request Mar 23, 2019

Support resilient values apple/swift-lldb#1393

Merged

adrian-prantl force-pushed the 48018240-irgen branch from 95c96e8 to 81018a7 Compare March 26, 2019 23:48

adrian-prantl force-pushed the 48018240-irgen branch from 81018a7 to 8b096bb Compare March 26, 2019 23:52

adrian-prantl commented Mar 26, 2019

View reviewed changes

rjmccall reviewed Mar 27, 2019

View reviewed changes

adrian-prantl force-pushed the 48018240-irgen branch from 8b096bb to e1f92cc Compare March 27, 2019 16:11

adrian-prantl merged commit e622ea6 into swiftlang:master Mar 27, 2019

Add a -public-linkage-for-metadata-accessors IRGen flag for LLDB. #23465

Add a -public-linkage-for-metadata-accessors IRGen flag for LLDB. #23465

Uh oh!

Conversation

adrian-prantl commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl commented Mar 21, 2019

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall commented Mar 21, 2019

Uh oh!

swift-ci commented Mar 21, 2019

Uh oh!

adrian-prantl commented Mar 22, 2019

Uh oh!

rjmccall commented Mar 22, 2019

Uh oh!

adrian-prantl commented Mar 26, 2019

Uh oh!

adrian-prantl Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

rjmccall Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Mar 27, 2019

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

rjmccall Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented Mar 27, 2019

Uh oh!

slavapestov commented Mar 27, 2019

Uh oh!

adrian-prantl commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl commented Mar 27, 2019

Uh oh!

adrian-prantl commented Mar 27, 2019

Uh oh!

slavapestov commented Mar 27, 2019

Uh oh!

adrian-prantl commented Mar 27, 2019

Uh oh!

adrian-prantl commented Mar 27, 2019

Uh oh!

slavapestov commented Mar 27, 2019

Uh oh!

jrose-apple commented Mar 27, 2019

Uh oh!

slavapestov commented Mar 27, 2019

Uh oh!

Uh oh!

adrian-prantl commented Mar 21, 2019 •

edited

Loading

adrian-prantl commented Mar 27, 2019 •

edited

Loading