Merge pull request swiftlang#36389 from atrick/poison-debug

[NFC] Poison references at -Onone after potentially shortening object lifetime

Author: atrick

docs/SIL.rst

@@ -3466,7 +3466,7 @@ debug_value
 
 ::
 
-  sil-instruction ::= debug_value sil-operand (',' debug-var-attr)*
+  sil-instruction ::= debug_value '[poison]'? sil-operand (',' debug-var-attr)*
 
   debug_value %1 : $Int
 
@@ -3488,6 +3488,15 @@ variable that is being described, including the name of the
 variable. For function and closure arguments ``argno`` is the number
 of the function argument starting with 1.
 
+If the '[poison]' flag is set, then all references within this debug
+value will be overwritten with a sentinel at this point in the
+program. This is used in debug builds when shortening non-trivial
+value lifetimes to ensure the debugger cannot inspect invalid
+memory. `debug_value` instructions with the poison flag are not
+generated until OSSA islowered. They are not expected to be serialized
+within the module, and the pipeline is not expected to do any
+significant code motion after lowering.
+
 debug_value_addr
 ````````````````
 

include/swift/SIL/SILBuilder.h

@@ -956,7 +956,8 @@ class SILBuilder {
   }
 
   DebugValueInst *createDebugValue(SILLocation Loc, SILValue src,
-                                   SILDebugVariable Var);
+                                   SILDebugVariable Var,
+                                   bool poisonRefs = false);
   DebugValueAddrInst *createDebugValueAddr(SILLocation Loc, SILValue src,
                                            SILDebugVariable Var);
 

include/swift/SIL/SILCloner.h

@@ -1280,7 +1280,8 @@ SILCloner<ImplClass>::visitDebugValueInst(DebugValueInst *Inst) {
   recordClonedInstruction(
       Inst, getBuilder().createDebugValue(Inst->getLoc(),
                                           getOpValue(Inst->getOperand()),
-                                          *Inst->getVarInfo()));
+                                          *Inst->getVarInfo(),
+                                          Inst->poisonRefs()));
 }
 template<typename ImplClass>
 void

include/swift/SIL/SILInstruction.h

@@ -4524,9 +4524,10 @@ class DebugValueInst final
   TailAllocatedDebugVariable VarInfo;
 
   DebugValueInst(SILDebugLocation DebugLoc, SILValue Operand,
-                 SILDebugVariable Var);
+                 SILDebugVariable Var, bool poisonRefs);
   static DebugValueInst *create(SILDebugLocation DebugLoc, SILValue Operand,
-                                SILModule &M, SILDebugVariable Var);
+                                SILModule &M, SILDebugVariable Var,
+                                bool poisonRefs);
 
   size_t numTrailingObjects(OverloadToken<char>) const { return 1; }
 
@@ -4538,6 +4539,21 @@ class DebugValueInst final
   Optional<SILDebugVariable> getVarInfo() const {
     return VarInfo.get(getDecl(), getTrailingObjects<char>());
   }
+
+  /// True if all references within this debug value will be overwritten with a
+  /// poison sentinel at this point in the program. This is used in debug builds
+  /// when shortening non-trivial value lifetimes to ensure the debugger cannot
+  /// inspect invalid memory. These are not generated until OSSA is
+  /// lowered. They are not expected to be serialized within the module, and the
+  /// debug pipeline is not expected to do any significant code motion after
+  /// OSSA lowering. It should not be necessary to model the poison operation as
+  /// a side effect, which would violate the rule that debug_values cannot
+  /// affect optimization.
+  bool poisonRefs() const { return SILNode::Bits.DebugValueInst.PoisonRefs; }
+
+  void setPoisonRefs(bool poisonRefs = true) {
+    SILNode::Bits.DebugValueInst.PoisonRefs = poisonRefs;
+  }
 };
 
 /// Define the start or update to a symbolic variable value (for address-only
@@ -7142,6 +7158,14 @@ class DestroyValueInst
   }
 
 public:
+  /// If true, then all references within the destroyed value will be
+  /// overwritten with a sentinel. This is used in debug builds when shortening
+  /// non-trivial value lifetimes to ensure the debugger cannot inspect invalid
+  /// memory. These semantics are part of the destroy_value instruction to
+  /// avoid representing use-after-destroy in OSSA form and so that OSSA
+  /// transformations keep the poison operation associated with the destroy
+  /// point. After OSSA, these are lowered to 'debug_values [poison]'
+  /// instructions, after which the Onone pipeline should avoid code motion.
   bool poisonRefs() const { return SILNode::Bits.DestroyValueInst.PoisonRefs; }
 
   void setPoisonRefs(bool poisonRefs = true) {

include/swift/SIL/SILNode.h

@@ -341,6 +341,9 @@ class alignas(8) SILNode {
   SWIFT_INLINE_BITFIELD(DestroyValueInst, NonValueInstruction, 1,
                         PoisonRefs : 1);
 
+  SWIFT_INLINE_BITFIELD(DebugValueInst, NonValueInstruction, 1,
+                        PoisonRefs : 1);
+
   SWIFT_INLINE_BITFIELD(EndCOWMutationInst, NonValueInstruction, 1,
     KeepUnique : 1
   );

include/swift/SILOptimizer/Utils/CanonicalOSSALifetime.h

@@ -93,6 +93,7 @@
 #ifndef SWIFT_SILOPTIMIZER_UTILS_CANONICALOSSALIFETIME_H
 #define SWIFT_SILOPTIMIZER_UTILS_CANONICALOSSALIFETIME_H
 
+#include "swift/Basic/SmallPtrSetVector.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SILOptimizer/Analysis/DominanceAnalysis.h"
 #include "swift/SILOptimizer/Analysis/NonLocalAccessBlockAnalysis.h"
@@ -123,13 +124,33 @@ class CanonicalOSSAConsumeInfo {
   /// that should be canonicalized separately.
   llvm::SmallDenseMap<SILBasicBlock *, CopyValueInst *, 4> persistentCopies;
 
+  /// The set of non-destroy consumes that need to be poisoned. This is
+  /// determined in two steps. First findOrInsertDestroyInBlock() checks if the
+  /// lifetime shrank within the block. Second rewriteCopies() checks if the
+  /// consume is in remnantLiveOutBlock(). Finally injectPoison() inserts new
+  /// copies and poison destroys for everything in this set.
+  SmallPtrSetVector<SILInstruction *, 4> needsPoisonConsumes;
+
 public:
   bool hasUnclaimedConsumes() const { return !finalBlockConsumes.empty(); }
 
   void clear() {
     finalBlockConsumes.clear();
     debugAfterConsume.clear();
     persistentCopies.clear();
+    needsPoisonConsumes.clear();
+  }
+
+  void recordNeedsPoison(SILInstruction *consume) {
+    needsPoisonConsumes.insert(consume);
+  }
+
+  bool needsPoison(SILInstruction *consume) const {
+    return needsPoisonConsumes.count(consume);
+  }
+
+  ArrayRef<SILInstruction *> getNeedsPoisonConsumes() const {
+    return needsPoisonConsumes.getArrayRef();
   }
 
   bool hasFinalConsumes() const { return !finalBlockConsumes.empty(); }
@@ -158,6 +179,11 @@ class CanonicalOSSAConsumeInfo {
     debugAfterConsume.push_back(dvi);
   }
 
+  void popDebugAfterConsume(DebugValueInst *dvi) {
+    if (!debugAfterConsume.empty() && debugAfterConsume.back() == dvi)
+      debugAfterConsume.pop_back();
+  }
+
   ArrayRef<DebugValueInst *> getDebugInstsAfterConsume() const {
     return debugAfterConsume;
   }
@@ -189,6 +215,9 @@ class CanonicalizeOSSALifetime {
   /// liveness may be pruned during canonicalization.
   bool pruneDebugMode;
 
+  /// If true, then new destroy_value instructions will be poison.
+  bool poisonRefsMode;
+
   NonLocalAccessBlockAnalysis *accessBlockAnalysis;
   // Lazily initialize accessBlocks only when
   // extendLivenessThroughOverlappingAccess is invoked.
@@ -232,16 +261,26 @@ class CanonicalizeOSSALifetime {
   /// ending". end_borrows do not end a liverange that may include owned copies.
   PrunedLiveness liveness;
 
+  /// remnantLiveOutBlocks are part of the original extended lifetime that are
+  /// not in canonical pruned liveness. There is a path from a PrunedLiveness
+  /// boundary to an original destroy that passes through a remnant block.
+  ///
+  /// These blocks would be equivalent to PrunedLiveness::LiveOut if
+  /// PrunedLiveness were recomputed using all original destroys as interesting
+  /// uses, minus blocks already marked PrunedLiveness::LiveOut. (Remnant blocks
+  /// may be in PrunedLiveness::LiveWithin).
+  SmallSetVector<SILBasicBlock *, 8> remnantLiveOutBlocks;
+
   /// Information about consuming instructions discovered in this caonical OSSA
   /// lifetime.
   CanonicalOSSAConsumeInfo consumes;
 
 public:
-  CanonicalizeOSSALifetime(bool pruneDebugMode,
+  CanonicalizeOSSALifetime(bool pruneDebugMode, bool poisonRefsMode,
                            NonLocalAccessBlockAnalysis *accessBlockAnalysis,
                            DominanceAnalysis *dominanceAnalysis,
                            DeadEndBlocks *deBlocks)
-      : pruneDebugMode(pruneDebugMode),
+      : pruneDebugMode(pruneDebugMode), poisonRefsMode(poisonRefsMode),
         accessBlockAnalysis(accessBlockAnalysis),
         dominanceAnalysis(dominanceAnalysis), deBlocks(deBlocks) {}
 
@@ -263,6 +302,7 @@ class CanonicalizeOSSALifetime {
     consumingBlocks.clear();
     debugValues.clear();
     liveness.clear();
+    remnantLiveOutBlocks.clear();
   }
 
   bool hasChanged() const { return changed; }
@@ -309,7 +349,12 @@ class CanonicalizeOSSALifetime {
 
   void findOrInsertDestroys();
 
+  void insertDestroyOnCFGEdge(SILBasicBlock *predBB, SILBasicBlock *succBB,
+                              bool needsPoison);
+
   void rewriteCopies();
+
+  void injectPoison();
 };
 
 } // end namespace swift

lib/IRGen/GenExistential.cpp

@@ -1958,6 +1958,14 @@ void irgen::emitMetatypeOfMetatype(IRGenFunction &IGF, Explosion &value,
   out.add(tablesAndValue.first);
 }
 
+Address
+irgen::emitClassExistentialValueAddress(IRGenFunction &IGF, Address existential,
+                                        SILType baseTy) {
+  assert(baseTy.isClassExistentialType());
+  auto &baseTI = IGF.getTypeInfo(baseTy).as<ClassExistentialTypeInfo>();
+  return baseTI.projectValue(IGF, existential);
+}
+
 /// Extract the instance pointer from a class existential value.
 llvm::Value *
 irgen::emitClassExistentialProjection(IRGenFunction &IGF,

lib/IRGen/GenExistential.h

@@ -99,6 +99,11 @@ namespace irgen {
       IRGenFunction &IGF, OpenedExistentialAccess accessKind, Address base,
       SILType existentialType, CanArchetypeType openedArchetype);
 
+  /// Return the address of the reference values within a class existential.
+  Address emitClassExistentialValueAddress(IRGenFunction &IGF,
+                                           Address existential,
+                                           SILType baseTy);
+
   /// Extract the instance pointer from a class existential value.
   ///
   /// \param openedArchetype If non-null, the archetype that will capture the