[DebugInfo] Add a verifier pass to find holes in lexical scopes. #13491

dcci · 2017-12-16T20:55:52Z

WIP, this can't be merged yet, it's just an experiment I and Adrian discussed yesterday.
OTOH, it apparently already caught a violation I'm investigating. It can't be enabled by default until at least we self-host with this on, but it seems like there's some value in being more strict here.

vedantk · 2017-12-18T19:37:16Z

lib/SIL/SILVerifier.cpp

+    AlreadySeenScopes.insert(LastSeenScope);
+    for (SILInstruction &SI : *BB) {
+      auto *DS = SI.getDebugScope();
+      if (!AlreadySeenScopes.count(DS)) {


nit: if (AlreadySeenScopes.insert(DS).second)?

Yes, definitely. I'll change it :)

JDevlieghere · 2017-12-19T00:32:17Z

I don't know much (~nothing) about the SILDebugScope representation, but I'm curious about the heuristic. If I read the code correctly, every instruction that doesn't introduce a new scope should have the same scope as the last one. Is it not possible that things are nested and that you re-encounter a previously seen scope?

dcci · 2017-12-19T02:25:30Z

@JDevlieghere From what I see this is not possible at -Onone, but the scenario you mention is definitely possible at higher optimizations level. @adrian-prantl can confirm/deny.

adrian-prantl · 2017-12-19T03:41:29Z

@JDevlieghere is correct. We need to add an additional check: if the "re-entered" scope is a (non-inlined) parent of the previous instruction, then this is okay.

adrian-prantl · 2017-12-19T03:42:54Z

For example:

{
  let i = 0; // scope 0
  if (cond) {
    let j = i // scope 1
  }
  let k = i + 1 // scope 0 and okay.
}

dcci · 2017-12-20T09:40:15Z

Updated after your comments. Thanks for your review!

It looks like this has found a case where -Onone breaks debug info.
This instruction breaks:

copy_addr %1 to [initialization] %2 : $*S, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:28, scope 2 // id: %5

Here we have something like:

alloc_stack -> scope 2
debug_value -> scope 1
debug_value -> scope 1
debug_value -> scope 2

SIL verification failed: Found an hole in debug lexical scopes: DS == LastSeenScope
In function:
sil_scope 1 { loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":200:13 parent @_T011ArrayAppend17appendFromGenericySay7ElementQzGz5array_x8sequencets8SequenceRzlF : $@convention(thin) <τ_0_0 where τ_0_0 : Sequence> (@inout Array<τ_0_0.Element>, @in τ_0_0) -> () }
sil_scope 2 { loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":204:1 parent 1 }

// appendFromGeneric<A>(array:sequence:)
sil [noinline] @_T011ArrayAppend17appendFromGenericySay7ElementQzGz5array_x8sequencets8SequenceRzlF : $@convention(thin) <S where S : Sequence> (@inout Array<S.Element>, @in S) -> () {
// %0                                             // users: %6, %3
// %1                                             // users: %10, %5, %4
bb0(%0 : $*Array<S.Element>, %1 : $*S):
  %2 = alloc_stack $S, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:28, scope 2 // users: %12, %8, %5
  debug_value_addr %0 : $*Array<S.Element>, var, name "array", argno 1, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":202:3, scope 1 // id: %3
  debug_value_addr %1 : $*S, let, name "sequence", argno 2, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":202:38, scope 1 // id: %4
  copy_addr %1 to [initialization] %2 : $*S, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:28, scope 2 // id: %5
  %6 = begin_access [modify] [static] %0 : $*Array<S.Element>, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:3, scope 2 // users: %9, %8
  // function_ref Array.append<A>(contentsOf:)
  %7 = function_ref @_T0Sa6appendyqd__10contentsOf_t7ElementQyd__Rszs8SequenceRd__lF : $@convention(method) <τ_0_0><τ_1_0 where τ_0_0 == τ_1_0.Element, τ_1_0 : Sequence> (@in τ_1_0, @inout Array<τ_0_0>) -> (), loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:9, scope 2 // user: %8
  %8 = apply %7<S.Element, S>(%2, %6) : $@convention(method) <τ_0_0><τ_1_0 where τ_0_0 == τ_1_0.Element, τ_1_0 : Sequence> (@in τ_1_0, @inout Array<τ_0_0>) -> (), loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:9, scope 2
  end_access %6 : $*Array<S.Element>, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:3, scope 2 // id: %9
  destroy_addr %1 : $*S, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":204:1, scope 2 // id: %10
  %11 = tuple (), loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":204:1, scope 2 // user: %13
  dealloc_stack %2 : $*S, loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":203:28, scope 2 // id: %12
  return %11 : $(), loc "/Users/dcci/work/swift/swift/benchmark/single-source/ArrayAppend.swift":204:1, scope 2 // id: %13
} // end sil function '_T011ArrayAppend17appendFromGenericySay7ElementQzGz5array_x8sequencets8SequenceRzlF'

Is this a valid scenario? From what I see scope 2 is a child of scope 1 so we shouldn't really escape from scope 1 and get to scope 2 in the same SIL Basic block

dcci · 2017-12-20T09:44:21Z

lib/SIL/SILVerifier.cpp

+
+    // Visit the instructions in the BB, to find holes in the lexical scopes.
+    // We use a fairly simple heuristic here (Explain heuristic).
+    llvm::DenseSet<const SILDebugScope *> AlreadySeenScopes;


This probably is growing enough that can be split in its own function. I've been doing this when I was at 30k feet and I didn't want to lose this snapshot :)

dcci · 2017-12-20T09:45:04Z

lib/SIL/SILVerifier.cpp

+      }
+      if (DS != LastSeenScope) {
+        llvm::dbgs() << "Broken instruction!\n"; SI.dump();
+        require(DS == LastSeenScope, "Found an hole in debug lexical scopes");


require is printing the wrong instruction (always the last one in the block). We probably need an overload where we pass the instruction to print.

dcci · 2017-12-20T09:46:04Z

tools/sil-opt/SILOpt.cpp

@@ -314,7 +314,7 @@ int main(int argc, char **argv) {
  // Setup the SIL Options.
  SILOptions &SILOpts = Invocation.getSILOptions();
  SILOpts.InlineThreshold = SILInlineThreshold;
-  SILOpts.VerifyAll = EnableSILVerifyAll;
+  SILOpts.VerifyAll = true;


This (and the other changes to LLVMOpt/CompilerInvocation seem to be required to trigger issues, otherwise we don't run the verifier after each pass). Of course this can't go in as is, but it seems to be valuable to keep this hack around in the PR for testing purposes.

JDevlieghere · 2017-12-20T11:06:44Z

lib/SIL/SILVerifier.cpp

+
+    // Visit the instructions in the BB, to find holes in the lexical scopes.
+    // We use a fairly simple heuristic here (Explain heuristic).
+    llvm::DenseSet<const SILDebugScope *> AlreadySeenScopes;


(nit) I'd move this down until after the additional checks, closer to the for loop.

JDevlieghere · 2017-12-20T11:07:06Z

lib/SIL/SILVerifier.cpp

+      if (!AlreadySeenScopes.count(DS)) {
+        AlreadySeenScopes.insert(DS);
+        LastSeenScope = DS;
+        continue;


If you always continue in the if-branch you might as well omit the else.

JDevlieghere

Two minor nits, but the heuristic/algorithm looks sound to me.

dcci · 2018-01-17T19:08:28Z

After fixing the issues that this verifier check exposes, this is ready for review/commit.
@adrian-prantl, can I ask you to take another fresh look?

vedantk

Looks good to me.

adrian-prantl · 2017-12-16T21:41:07Z

lib/SIL/SILVerifier.cpp

+      return;
+    SILInstruction &First = *BB->begin();
+    const SILDebugScope *LastSeenScope = First.getDebugScope();
+    AlreadySeenScopes.insert(LastSeenScope);


This doesn't yet handle inlined scopes. I think the right way to handle them here is to insert the scope of the inlined call site into the map. This way all inlined instructions belong to the same scope (the scope of the apply instruction that was inlined).

We also need to to restrict this to only run at -ONone. Non-mandatory optimizations may legitimately hoist sink or merge instructions.

Can the inlined scope be handled as a follow up I assume?
About -Onone, I think this code should take care of it:

// This check only makes sense at -Onone. Optimizations, // e.g. inlining, can move scopes around. llvm::DenseSet<const SILDebugScope *> AlreadySeenScopes; if (BB->getParent()->getEffectiveOptimizationMode() != OptimizationMode::NoOptimization) return;

adrian-prantl · 2017-12-16T21:46:15Z

lib/SIL/SILVerifier.cpp

+        LastSeenScope = DS;
+        continue;
+      }
+      require(DS == LastSeenScope, "Found an hole in debug lexical scopes");


The check looks good!
perhaps: "Basic block contains a non-contiguous lexical scope at -Onone"

Good idea, done!

adrian-prantl · 2018-01-17T19:12:17Z

include/swift/SIL/SILDebugScope.h

@@ -64,6 +64,10 @@ class SILDebugScope : public SILAllocated<SILDebugScope> {
  /// into.
  SILFunction *getParentFunction() const;

+  PointerUnion<const SILDebugScope *, SILFunction *> getParent() const {


I think we should either have an accessor, or make the field public, but not both.

adrian-prantl · 2018-01-17T19:44:34Z

lib/SIL/SILVerifier.cpp

@@ -4338,6 +4342,76 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
    }
  }

+  // This pass verifies that there are no hole in debug scopes at -Onone.


adrian-prantl · 2018-01-17T19:45:15Z

lib/SIL/SILVerifier.cpp

@@ -56,6 +56,10 @@ static llvm::cl::opt<bool> AbortOnFailure(
                              "verify-abort-on-failure",
                              llvm::cl::init(true));

+static llvm::cl::opt<bool> VerifyDIHoles(


Is this option really necessary, shouldn't it just always be on?

I think this is good while we're transitioning, so that if something breaks, we can just turn the option off and investigate rather than reverting the whole commit.

dcci · 2018-01-17T20:42:34Z

Addressed comments & replied. Thanks for your reviews, @adrian-prantl/ @vedantk !

dcci · 2018-01-17T20:48:22Z

@swift-ci please test

swift-ci · 2018-01-17T20:48:46Z

Build failed
Swift Test OS X Platform
Git Sha - 61097f52ffd9f65b98389e2fe29f9fc7182c748e

swift-ci · 2018-01-17T20:49:55Z

Build failed
Swift Test Linux Platform
Git Sha - 61097f52ffd9f65b98389e2fe29f9fc7182c748e

dcci · 2018-01-17T20:59:06Z

@swift-ci please test

swift-ci · 2018-01-17T20:59:24Z

Build failed
Swift Test OS X Platform
Git Sha - 0adfd6e79b0f194d4ab8f9c4baf5cc64eca33d49

swift-ci · 2018-01-17T21:00:52Z

Build failed
Swift Test Linux Platform
Git Sha - 0adfd6e79b0f194d4ab8f9c4baf5cc64eca33d49

dcci · 2018-01-17T23:22:04Z

Apparently this found a bug in swiftpm :)

1.	While running pass #557 SILModuleTransform "MandatoryInlining".
2.	While verifying SIL function "@$S5Basic25LocalFileOutputByteStreamC_13closeOnDeinitAcA12AbsolutePathV_SbtKcfc".
 for 'init(_:closeOnDeinit:)' at /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swiftpm/Sources/Basic/OutputByteStream.swift:657:12

vedantk · 2018-01-17T23:23:38Z

Just a heads-up that you'll want to run "AT swift-ci Please test source compatibility" before checking this in. That contains a large suite of swift projects which we'll want to run the verifier over.

vedantk · 2018-01-17T23:24:00Z

In fact
@swift-ci Please test source compatibility

This found a bunch of issues where we didn't properly preserve debug infos which have been fixed over the course of the past two weeks. The basic idea is we want to make sure that within a BB we never have `holes` in debug scopes, i.e. if we enter a scope, one of the two holds 1) We never visited that scope before 2) We're re-entering a scope that is an ancestor of the scope currently leaving. This is needed to correctly model nested scopes, e.g. { let i = 0; // scope 0 if (cond) { let j = i // scope 1 } let k = i + 1 // scope 0 and okay. } I also checked in a `cl::opt`, so that if this breaks something, it can be conveniently disabled. This is currently not enabled as we flesh out the last violations. <rdar://problem/35816212>

dcci · 2018-01-17T23:26:05Z

@swift-ci please test and merge

dcci · 2018-01-17T23:26:57Z

@swift-ci Please test source compatibility

shahmishal · 2018-01-17T23:33:16Z

@swift-ci test source compatibility

dcci · 2018-01-17T23:44:20Z

I'm currently merging this with the support disabled, as this is breaks a lot of code in the wild probably (and unfortunately, I'd add). I plan to write an RFC to send to swift-dev so that people can check & report bugs, and then I'll enable this after a grace period.

dcci · 2018-01-18T02:08:40Z

@swift-ci please smoke test and merge

dcci · 2018-01-18T06:51:52Z

@swift-ci test and merge

dcci requested review from vedantk, JDevlieghere, adrian-prantl and fredriss December 16, 2017 20:55

vedantk reviewed Dec 18, 2017

View reviewed changes

dcci force-pushed the lexicalscopes branch from 213537e to b00ae08 Compare December 20, 2017 09:43

dcci commented Dec 20, 2017

View reviewed changes

JDevlieghere reviewed Dec 20, 2017

View reviewed changes

dcci force-pushed the lexicalscopes branch 5 times, most recently from fe2b6e2 to 61097f5 Compare January 17, 2018 19:07

vedantk approved these changes Jan 17, 2018

View reviewed changes

adrian-prantl requested changes Jan 17, 2018

View reviewed changes

dcci changed the title ~~[DebugInfo/WIP] Add a verifier pass to find holes in lexical scopes.~~ [DebugInfo] Add a verifier pass to find holes in lexical scopes. Jan 17, 2018

dcci force-pushed the lexicalscopes branch from 61097f5 to 0adfd6e Compare January 17, 2018 20:42

dcci force-pushed the lexicalscopes branch from 0adfd6e to 99711a5 Compare January 17, 2018 20:57

adrian-prantl approved these changes Jan 17, 2018

View reviewed changes

dcci force-pushed the lexicalscopes branch from 99711a5 to 2006691 Compare January 17, 2018 23:25

dcci force-pushed the lexicalscopes branch from 2006691 to a4dba76 Compare January 17, 2018 23:25

swift-ci merged commit d01c9fa into swiftlang:master Jan 18, 2018

dcci mannequin mentioned this pull request Dec 12, 2022

[SR-6700] [DebugInfo] SIL alloc_stack hoisting doesn't preserve DI correctly #49249

Closed

[DebugInfo] Add a verifier pass to find holes in lexical scopes. #13491

[DebugInfo] Add a verifier pass to find holes in lexical scopes. #13491

Uh oh!

Conversation

dcci commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere commented Dec 19, 2017

Uh oh!

dcci commented Dec 19, 2017

Uh oh!

adrian-prantl commented Dec 19, 2017

Uh oh!

adrian-prantl commented Dec 19, 2017

Uh oh!

dcci commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

dcci commented Jan 17, 2018

Uh oh!

vedantk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcci commented Jan 17, 2018

Uh oh!

dcci commented Jan 17, 2018

Uh oh!

swift-ci commented Jan 17, 2018

Uh oh!

swift-ci commented Jan 17, 2018

Uh oh!

dcci commented Jan 17, 2018

Uh oh!

swift-ci commented Jan 17, 2018

Uh oh!

swift-ci commented Jan 17, 2018

Uh oh!

dcci commented Jan 17, 2018

Uh oh!

vedantk commented Jan 17, 2018

dcci commented Dec 20, 2017 •

edited

Loading

JDevlieghere Dec 20, 2017 •

edited

Loading