Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows SEH] Fix crash on empty seh block #107031

Merged
merged 8 commits into from
Sep 4, 2024
Merged

Conversation

R-Goc
Copy link
Contributor

@R-Goc R-Goc commented Sep 3, 2024

Fixes #105813 and #106915.
Adds a check for the end of the iterator, which can be a sentinel.
The issue was introduced in 0efe111 from what I can see, so along with the introduction of /EHa support.

Copy link

github-actions bot commented Sep 3, 2024

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Sep 3, 2024
@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

I'm aware that clang-format may have to be run on this, not sure why my editor changed anything.

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 3, 2024

@llvm/pr-subscribers-llvm-selectiondag

Author: None (R-Goc)

Changes

Fixes #105813 and #106915.
Adds a check for the end of the iterator, which can be a sentinel.


Patch is 53.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/107031.diff

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+300-265)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index b37e54d66ddf51..22a4cb04d717ef 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -121,7 +121,8 @@ STATISTIC(NumFastIselFailures, "Number of instructions fast isel failed on");
 STATISTIC(NumFastIselSuccess, "Number of instructions fast isel selected");
 STATISTIC(NumFastIselBlocks, "Number of blocks selected entirely by fast isel");
 STATISTIC(NumDAGBlocks, "Number of blocks selected using DAG");
-STATISTIC(NumDAGIselRetries,"Number of times dag isel has to try another path");
+STATISTIC(NumDAGIselRetries,
+          "Number of times dag isel has to try another path");
 STATISTIC(NumEntryBlocks, "Number of entry blocks encountered");
 STATISTIC(NumFastIselFailLowerArguments,
           "Number of entry blocks where fast isel failed to lower arguments");
@@ -139,23 +140,22 @@ static cl::opt<bool> EnableFastISelFallbackReport(
     cl::desc("Emit a diagnostic when \"fast\" instruction selection "
              "falls back to SelectionDAG."));
 
-static cl::opt<bool>
-UseMBPI("use-mbpi",
-        cl::desc("use Machine Branch Probability Info"),
-        cl::init(true), cl::Hidden);
+static cl::opt<bool> UseMBPI("use-mbpi",
+                             cl::desc("use Machine Branch Probability Info"),
+                             cl::init(true), cl::Hidden);
 
 #ifndef NDEBUG
-static cl::opt<std::string>
-FilterDAGBasicBlockName("filter-view-dags", cl::Hidden,
-                        cl::desc("Only display the basic block whose name "
-                                 "matches this for all view-*-dags options"));
-static cl::opt<bool>
-ViewDAGCombine1("view-dag-combine1-dags", cl::Hidden,
-          cl::desc("Pop up a window to show dags before the first "
-                   "dag combine pass"));
+static cl::opt<std::string> FilterDAGBasicBlockName(
+    "filter-view-dags", cl::Hidden,
+    cl::desc("Only display the basic block whose name "
+             "matches this for all view-*-dags options"));
 static cl::opt<bool>
-ViewLegalizeTypesDAGs("view-legalize-types-dags", cl::Hidden,
-          cl::desc("Pop up a window to show dags before legalize types"));
+    ViewDAGCombine1("view-dag-combine1-dags", cl::Hidden,
+                    cl::desc("Pop up a window to show dags before the first "
+                             "dag combine pass"));
+static cl::opt<bool> ViewLegalizeTypesDAGs(
+    "view-legalize-types-dags", cl::Hidden,
+    cl::desc("Pop up a window to show dags before legalize types"));
 static cl::opt<bool>
     ViewDAGCombineLT("view-dag-combine-lt-dags", cl::Hidden,
                      cl::desc("Pop up a window to show dags before the post "
@@ -164,18 +164,18 @@ static cl::opt<bool>
     ViewLegalizeDAGs("view-legalize-dags", cl::Hidden,
                      cl::desc("Pop up a window to show dags before legalize"));
 static cl::opt<bool>
-ViewDAGCombine2("view-dag-combine2-dags", cl::Hidden,
-          cl::desc("Pop up a window to show dags before the second "
-                   "dag combine pass"));
-static cl::opt<bool>
-ViewISelDAGs("view-isel-dags", cl::Hidden,
-          cl::desc("Pop up a window to show isel dags as they are selected"));
-static cl::opt<bool>
-ViewSchedDAGs("view-sched-dags", cl::Hidden,
-          cl::desc("Pop up a window to show sched dags as they are processed"));
-static cl::opt<bool>
-ViewSUnitDAGs("view-sunit-dags", cl::Hidden,
-      cl::desc("Pop up a window to show SUnit dags after they are processed"));
+    ViewDAGCombine2("view-dag-combine2-dags", cl::Hidden,
+                    cl::desc("Pop up a window to show dags before the second "
+                             "dag combine pass"));
+static cl::opt<bool> ViewISelDAGs(
+    "view-isel-dags", cl::Hidden,
+    cl::desc("Pop up a window to show isel dags as they are selected"));
+static cl::opt<bool> ViewSchedDAGs(
+    "view-sched-dags", cl::Hidden,
+    cl::desc("Pop up a window to show sched dags as they are processed"));
+static cl::opt<bool> ViewSUnitDAGs(
+    "view-sunit-dags", cl::Hidden,
+    cl::desc("Pop up a window to show SUnit dags after they are processed"));
 #else
 static const bool ViewDAGCombine1 = false, ViewLegalizeTypesDAGs = false,
                   ViewDAGCombineLT = false, ViewLegalizeDAGs = false,
@@ -193,7 +193,9 @@ static const bool ViewDAGCombine1 = false, ViewLegalizeTypesDAGs = false,
     }                                                                          \
   } while (false)
 #else
-#define ISEL_DUMP(X) do { } while (false)
+#define ISEL_DUMP(X)                                                           \
+  do {                                                                         \
+  } while (false)
 #endif
 
 //===---------------------------------------------------------------------===//
@@ -211,14 +213,13 @@ MachinePassRegistry<RegisterScheduler::FunctionPassCtor>
 //===---------------------------------------------------------------------===//
 static cl::opt<RegisterScheduler::FunctionPassCtor, false,
                RegisterPassParser<RegisterScheduler>>
-ISHeuristic("pre-RA-sched",
-            cl::init(&createDefaultScheduler), cl::Hidden,
-            cl::desc("Instruction schedulers available (before register"
-                     " allocation):"));
+    ISHeuristic("pre-RA-sched", cl::init(&createDefaultScheduler), cl::Hidden,
+                cl::desc("Instruction schedulers available (before register"
+                         " allocation):"));
 
 static RegisterScheduler
-defaultListDAGScheduler("default", "Best scheduler for the target",
-                        createDefaultScheduler);
+    defaultListDAGScheduler("default", "Best scheduler for the target",
+                            createDefaultScheduler);
 
 static bool dontUseFastISelFor(const Function &Fn) {
   // Don't enable FastISel for functions with swiftasync Arguments.
@@ -232,83 +233,82 @@ static bool dontUseFastISelFor(const Function &Fn) {
 
 namespace llvm {
 
-  //===--------------------------------------------------------------------===//
-  /// This class is used by SelectionDAGISel to temporarily override
-  /// the optimization level on a per-function basis.
-  class OptLevelChanger {
-    SelectionDAGISel &IS;
-    CodeGenOptLevel SavedOptLevel;
-    bool SavedFastISel;
-
-  public:
-    OptLevelChanger(SelectionDAGISel &ISel, CodeGenOptLevel NewOptLevel)
-        : IS(ISel) {
-      SavedOptLevel = IS.OptLevel;
-      SavedFastISel = IS.TM.Options.EnableFastISel;
-      if (NewOptLevel != SavedOptLevel) {
-        IS.OptLevel = NewOptLevel;
-        IS.TM.setOptLevel(NewOptLevel);
-        LLVM_DEBUG(dbgs() << "\nChanging optimization level for Function "
-                          << IS.MF->getFunction().getName() << "\n");
-        LLVM_DEBUG(dbgs() << "\tBefore: -O" << static_cast<int>(SavedOptLevel)
-                          << " ; After: -O" << static_cast<int>(NewOptLevel)
-                          << "\n");
-        if (NewOptLevel == CodeGenOptLevel::None)
-          IS.TM.setFastISel(IS.TM.getO0WantsFastISel());
-      }
-      if (dontUseFastISelFor(IS.MF->getFunction()))
-        IS.TM.setFastISel(false);
-      LLVM_DEBUG(
-          dbgs() << "\tFastISel is "
-                 << (IS.TM.Options.EnableFastISel ? "enabled" : "disabled")
-                 << "\n");
-    }
+//===--------------------------------------------------------------------===//
+/// This class is used by SelectionDAGISel to temporarily override
+/// the optimization level on a per-function basis.
+class OptLevelChanger {
+  SelectionDAGISel &IS;
+  CodeGenOptLevel SavedOptLevel;
+  bool SavedFastISel;
 
-    ~OptLevelChanger() {
-      if (IS.OptLevel == SavedOptLevel)
-        return;
-      LLVM_DEBUG(dbgs() << "\nRestoring optimization level for Function "
+public:
+  OptLevelChanger(SelectionDAGISel &ISel, CodeGenOptLevel NewOptLevel)
+      : IS(ISel) {
+    SavedOptLevel = IS.OptLevel;
+    SavedFastISel = IS.TM.Options.EnableFastISel;
+    if (NewOptLevel != SavedOptLevel) {
+      IS.OptLevel = NewOptLevel;
+      IS.TM.setOptLevel(NewOptLevel);
+      LLVM_DEBUG(dbgs() << "\nChanging optimization level for Function "
                         << IS.MF->getFunction().getName() << "\n");
-      LLVM_DEBUG(dbgs() << "\tBefore: -O" << static_cast<int>(IS.OptLevel)
-                        << " ; After: -O" << static_cast<int>(SavedOptLevel) << "\n");
-      IS.OptLevel = SavedOptLevel;
-      IS.TM.setOptLevel(SavedOptLevel);
-      IS.TM.setFastISel(SavedFastISel);
+      LLVM_DEBUG(dbgs() << "\tBefore: -O" << static_cast<int>(SavedOptLevel)
+                        << " ; After: -O" << static_cast<int>(NewOptLevel)
+                        << "\n");
+      if (NewOptLevel == CodeGenOptLevel::None)
+        IS.TM.setFastISel(IS.TM.getO0WantsFastISel());
     }
-  };
+    if (dontUseFastISelFor(IS.MF->getFunction()))
+      IS.TM.setFastISel(false);
+    LLVM_DEBUG(dbgs() << "\tFastISel is "
+                      << (IS.TM.Options.EnableFastISel ? "enabled" : "disabled")
+                      << "\n");
+  }
 
-  //===--------------------------------------------------------------------===//
-  /// createDefaultScheduler - This creates an instruction scheduler appropriate
-  /// for the target.
-  ScheduleDAGSDNodes *createDefaultScheduler(SelectionDAGISel *IS,
-                                             CodeGenOptLevel OptLevel) {
-    const TargetLowering *TLI = IS->TLI;
-    const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
-
-    // Try first to see if the Target has its own way of selecting a scheduler
-    if (auto *SchedulerCtor = ST.getDAGScheduler(OptLevel)) {
-      return SchedulerCtor(IS, OptLevel);
-    }
+  ~OptLevelChanger() {
+    if (IS.OptLevel == SavedOptLevel)
+      return;
+    LLVM_DEBUG(dbgs() << "\nRestoring optimization level for Function "
+                      << IS.MF->getFunction().getName() << "\n");
+    LLVM_DEBUG(dbgs() << "\tBefore: -O" << static_cast<int>(IS.OptLevel)
+                      << " ; After: -O" << static_cast<int>(SavedOptLevel)
+                      << "\n");
+    IS.OptLevel = SavedOptLevel;
+    IS.TM.setOptLevel(SavedOptLevel);
+    IS.TM.setFastISel(SavedFastISel);
+  }
+};
 
-    if (OptLevel == CodeGenOptLevel::None ||
-        (ST.enableMachineScheduler() && ST.enableMachineSchedDefaultSched()) ||
-        TLI->getSchedulingPreference() == Sched::Source)
-      return createSourceListDAGScheduler(IS, OptLevel);
-    if (TLI->getSchedulingPreference() == Sched::RegPressure)
-      return createBURRListDAGScheduler(IS, OptLevel);
-    if (TLI->getSchedulingPreference() == Sched::Hybrid)
-      return createHybridListDAGScheduler(IS, OptLevel);
-    if (TLI->getSchedulingPreference() == Sched::VLIW)
-      return createVLIWDAGScheduler(IS, OptLevel);
-    if (TLI->getSchedulingPreference() == Sched::Fast)
-      return createFastDAGScheduler(IS, OptLevel);
-    if (TLI->getSchedulingPreference() == Sched::Linearize)
-      return createDAGLinearizer(IS, OptLevel);
-    assert(TLI->getSchedulingPreference() == Sched::ILP &&
-           "Unknown sched type!");
-    return createILPListDAGScheduler(IS, OptLevel);
+//===--------------------------------------------------------------------===//
+/// createDefaultScheduler - This creates an instruction scheduler appropriate
+/// for the target.
+ScheduleDAGSDNodes *createDefaultScheduler(SelectionDAGISel *IS,
+                                           CodeGenOptLevel OptLevel) {
+  const TargetLowering *TLI = IS->TLI;
+  const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
+
+  // Try first to see if the Target has its own way of selecting a scheduler
+  if (auto *SchedulerCtor = ST.getDAGScheduler(OptLevel)) {
+    return SchedulerCtor(IS, OptLevel);
   }
 
+  if (OptLevel == CodeGenOptLevel::None ||
+      (ST.enableMachineScheduler() && ST.enableMachineSchedDefaultSched()) ||
+      TLI->getSchedulingPreference() == Sched::Source)
+    return createSourceListDAGScheduler(IS, OptLevel);
+  if (TLI->getSchedulingPreference() == Sched::RegPressure)
+    return createBURRListDAGScheduler(IS, OptLevel);
+  if (TLI->getSchedulingPreference() == Sched::Hybrid)
+    return createHybridListDAGScheduler(IS, OptLevel);
+  if (TLI->getSchedulingPreference() == Sched::VLIW)
+    return createVLIWDAGScheduler(IS, OptLevel);
+  if (TLI->getSchedulingPreference() == Sched::Fast)
+    return createFastDAGScheduler(IS, OptLevel);
+  if (TLI->getSchedulingPreference() == Sched::Linearize)
+    return createDAGLinearizer(IS, OptLevel);
+  assert(TLI->getSchedulingPreference() == Sched::ILP && "Unknown sched type!");
+  return createILPListDAGScheduler(IS, OptLevel);
+}
+
 } // end namespace llvm
 
 MachineBasicBlock *
@@ -316,8 +316,8 @@ TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
                                             MachineBasicBlock *MBB) const {
 #ifndef NDEBUG
   dbgs() << "If a target marks an instruction with "
-          "'usesCustomInserter', it must implement "
-          "TargetLowering::EmitInstrWithCustomInserter!\n";
+            "'usesCustomInserter', it must implement "
+            "TargetLowering::EmitInstrWithCustomInserter!\n";
 #endif
   llvm_unreachable(nullptr);
 }
@@ -396,7 +396,7 @@ SelectionDAGISel::~SelectionDAGISel() {
 void SelectionDAGISelLegacy::getAnalysisUsage(AnalysisUsage &AU) const {
   CodeGenOptLevel OptLevel = Selector->OptLevel;
   if (OptLevel != CodeGenOptLevel::None)
-      AU.addRequired<AAResultsWrapperPass>();
+    AU.addRequired<AAResultsWrapperPass>();
   AU.addRequired<GCModuleInfo>();
   AU.addRequired<StackProtector>();
   AU.addPreserved<GCModuleInfo>();
@@ -406,14 +406,14 @@ void SelectionDAGISelLegacy::getAnalysisUsage(AnalysisUsage &AU) const {
 #endif
   AU.addRequired<AssumptionCacheTracker>();
   if (UseMBPI && OptLevel != CodeGenOptLevel::None)
-      AU.addRequired<BranchProbabilityInfoWrapperPass>();
+    AU.addRequired<BranchProbabilityInfoWrapperPass>();
   AU.addRequired<ProfileSummaryInfoWrapperPass>();
   // AssignmentTrackingAnalysis only runs if assignment tracking is enabled for
   // the module.
   AU.addRequired<AssignmentTrackingAnalysis>();
   AU.addPreserved<AssignmentTrackingAnalysis>();
   if (OptLevel != CodeGenOptLevel::None)
-      LazyBlockFrequencyInfoPass::getLazyBFIAnalysisUsage(AU);
+    LazyBlockFrequencyInfoPass::getLazyBFIAnalysisUsage(AU);
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
@@ -658,7 +658,7 @@ bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
 
   // Insert copies in the entry block and the return blocks.
   if (FuncInfo->SplitCSR) {
-    SmallVector<MachineBasicBlock*, 4> Returns;
+    SmallVector<MachineBasicBlock *, 4> Returns;
     // Collect all the return blocks.
     for (MachineBasicBlock &MBB : mf) {
       if (!MBB.succ_empty())
@@ -857,7 +857,8 @@ void SelectionDAGISel::SelectBasicBlock(BasicBlock::const_iterator Begin,
   // Lower the instructions. If a call is emitted as a tail call, cease emitting
   // nodes for this block. If an instruction is elided, don't emit it, but do
   // handle any debug-info attached to it.
-  for (BasicBlock::const_iterator I = Begin; I != End && !SDB->HasTailCall; ++I) {
+  for (BasicBlock::const_iterator I = Begin; I != End && !SDB->HasTailCall;
+       ++I) {
     if (!ElidedArgCopyInstrs.count(&*I))
       SDB->visit(*I);
     else
@@ -876,7 +877,7 @@ void SelectionDAGISel::SelectBasicBlock(BasicBlock::const_iterator Begin,
 
 void SelectionDAGISel::ComputeLiveOutVRegInfo() {
   SmallPtrSet<SDNode *, 16> Added;
-  SmallVector<SDNode*, 128> Worklist;
+  SmallVector<SDNode *, 128> Worklist;
 
   Worklist.push_back(CurDAG->getRoot().getNode());
   Added.insert(CurDAG->getRoot().getNode());
@@ -922,9 +923,9 @@ void SelectionDAGISel::CodeGenAndEmitDAG() {
   CurDAG->NewNodesMustHaveLegalTypes = false;
 
 #ifndef NDEBUG
-  MatchFilterBB = (FilterDAGBasicBlockName.empty() ||
-                   FilterDAGBasicBlockName ==
-                       FuncInfo->MBB->getBasicBlock()->getName());
+  MatchFilterBB =
+      (FilterDAGBasicBlockName.empty() ||
+       FilterDAGBasicBlockName == FuncInfo->MBB->getBasicBlock()->getName());
 #endif
 #ifdef NDEBUG
   if (ViewDAGCombine1 || ViewLegalizeTypesDAGs || ViewDAGCombineLT ||
@@ -1175,7 +1176,7 @@ class ISelUpdater : public SelectionDAG::DAGUpdateListener {
 
 public:
   ISelUpdater(SelectionDAG &DAG, SelectionDAG::allnodes_iterator &isp)
-    : SelectionDAG::DAGUpdateListener(DAG), ISelPosition(isp) {}
+      : SelectionDAG::DAGUpdateListener(DAG), ISelPosition(isp) {}
 
   /// NodeDeleted - Handle nodes deleted from the graph. If the node being
   /// deleted is the current ISelPosition node, update ISelPosition.
@@ -1267,7 +1268,7 @@ void SelectionDAGISel::DoInstructionSelection() {
     // a reference to the root node, preventing it from being deleted,
     // and tracking any changes of the root.
     HandleSDNode Dummy(CurDAG->getRoot());
-    SelectionDAG::allnodes_iterator ISelPosition (CurDAG->getRoot().getNode());
+    SelectionDAG::allnodes_iterator ISelPosition(CurDAG->getRoot().getNode());
     ++ISelPosition;
 
     // Make sure that ISelPosition gets properly updated when nodes are deleted
@@ -1339,8 +1340,8 @@ void SelectionDAGISel::DoInstructionSelection() {
           ActionVT = Node->getValueType(0);
           break;
         }
-        if (TLI->getOperationAction(Node->getOpcode(), ActionVT)
-            == TargetLowering::Expand)
+        if (TLI->getOperationAction(Node->getOpcode(), ActionVT) ==
+            TargetLowering::Expand)
           Node = CurDAG->mutateStrictFPToFP(Node);
       }
 
@@ -1439,8 +1440,7 @@ bool SelectionDAGISel::PrepareEHLandingPad() {
   MCSymbol *Label = MF->addLandingPad(MBB);
 
   const MCInstrDesc &II = TII->get(TargetOpcode::EH_LABEL);
-  BuildMI(*MBB, FuncInfo->InsertPt, SDB->getCurDebugLoc(), II)
-    .addSym(Label);
+  BuildMI(*MBB, FuncInfo->InsertPt, SDB->getCurDebugLoc(), II).addSym(Label);
 
   // If the unwinder does not preserve all registers, ensure that the
   // function marks the clobbered registers as used.
@@ -1476,6 +1476,11 @@ void SelectionDAGISel::reportIPToStateForBlocks(MachineFunction *MF) {
     if (BB->getFirstMayFaultInst()) {
       // Report IP range only for blocks with Faulty inst
       auto MBBb = MBB.getFirstNonPHI();
+
+      // Avoids attempting to dereference a sentintel which fails an assert
+      if (MBBb == MBB.instr_end())
+        continue;
+
       MachineInstr *MIb = &*MBBb;
       if (MIb->isTerminator())
         continue;
@@ -1627,7 +1632,7 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
     FastIS = TLI->createFastISel(*FuncInfo, LibInfo);
   }
 
-  ReversePostOrderTraversal<const Function*> RPOT(&Fn);
+  ReversePostOrderTraversal<const Function *> RPOT(&Fn);
 
   // Lower arguments up front. An RPO iteration always visits the entry block
   // first.
@@ -1651,8 +1656,7 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
       ++NumFastIselFailLowerArguments;
 
       OptimizationRemarkMissed R("sdagisel", "FastISelFailure",
-                                 Fn.getSubprogram(),
-                                 &Fn.getEntryBlock());
+                                 Fn.getSubprogram(), &Fn.getEntryBlock());
       R << "FastISel didn't lower all arguments: "
         << ore::NV("Prototype", Fn.getFunctionType());
       reportFastISelFailure(*MF, *ORE, R, EnableFastISelAbort > 1);
@@ -1919,8 +1923,7 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
   SDB->SPDescriptor.resetPerFunctionState();
 }
 
-void
-SelectionDAGISel::FinishBasicBlock() {
+void SelectionDAGISel::FinishBasicBlock() {
   LLVM_DEBUG(dbgs() << "Total amount of phi nodes to update: "
                     << FuncInfo->PHINodesToUpdate.size() << "\n";
              for (unsigned i = 0, e = FuncInfo->PHINodesToUpdate.size(); i != e;
@@ -1947,8 +1950,7 @@ SelectionDAGISel::FinishBasicBlock() {
 
     // Add load and check to the basicblock.
     FuncInfo->MBB = ParentMBB;
-    FuncInfo->InsertPt =
-        findSplitPointForStackProtector(ParentMBB, *TII);
+    FuncInfo->InsertPt = findSplitPointForStackProtector(ParentMBB, *TII);
     SDB->visitSPDescriptorParent(SDB->SPDescriptor, ParentMBB);
     CurDAG->setRoot(SDB-...
[truncated]

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

Thanks to @glebov-andrey, @asl and @jrtc27.

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 3, 2024

It looks like you ran clang-format on the file rather than the diff?

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 3, 2024

This also would ideally have a test case.

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

Would adding a test to llvm-test-suite be fine?

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 3, 2024

Not really. Ideally there would be a minimal reproducer in llvm/test.

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

I'm having trouble getting the reproducer to be an .ll or .bc file. It seems even less deterministic in that form. For example debug llc doesn't hang on it. Bugpoint fails to emit a proper file as well, and emits an invalid module. Currently there are 3 known reproductions, 2 of which are very delicate and fail with seemingly unrelated options. The one that basically always fails is a 350k loc part of protobufs. But even that seems to run fine with debug llc.

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 3, 2024

Reduced: https://godbolt.org/z/7hhsxeqj5

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

Out of curiosity how did you do it? Manually?

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 3, 2024

I ran the C++ through creduce to cut it down to something llvm-reduce wouldn't crash on (it seemed to quickly bail on an invoke IIRC, perhaps not adapted to Windows-style exceptions), and then ran it through llvm-reduce to get a minimal IR test.

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

Okay. In terms of naming I'm not sure. Does wineh-unclosed-scope.ll work? Or maybe wineh-empty-seh-scope.ll .Seems like tests related to eh land in llvm/test/CodeGen/WinEH

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 3, 2024

What is left to do to get this merged?

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 4, 2024

Should I rebase and squash now?

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 4, 2024

A lot of commits seem to be waiting for linux workers. Is this normal?

@arsenm
Copy link
Contributor

arsenm commented Sep 4, 2024

A lot of commits seem to be waiting for linux workers. Is this normal?

Yes

R-Goc and others added 2 commits September 4, 2024 18:10
Co-authored-by: Matt Arsenault <arsenm2@gmail.com>
@arsenm
Copy link
Contributor

arsenm commented Sep 4, 2024

Can you reword the commit title to be more specific about empty blocks

@R-Goc R-Goc changed the title [Windows SEH] fix failed assert and crash [Windows SEH] fix crash on empty seh block Sep 4, 2024
@R-Goc R-Goc changed the title [Windows SEH] fix crash on empty seh block [Windows SEH] Fix crash on empty seh block Sep 4, 2024
@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 4, 2024

Would this be a good candidate to backport to LLVM 19?

@arsenm
Copy link
Contributor

arsenm commented Sep 4, 2024

Would this be a good candidate to backport to LLVM 19?

Yes

@arsenm arsenm merged commit 2e0ded3 into llvm:main Sep 4, 2024
6 of 7 checks passed
Copy link

github-actions bot commented Sep 4, 2024

@R-Goc Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@R-Goc
Copy link
Contributor Author

R-Goc commented Sep 5, 2024

/cherry-pick 2e0ded3

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 5, 2024

/cherry-pick 2e0ded3

Error: Command failed due to missing milestone.

@R-Goc R-Goc deleted the fix-Windows-SEH branch September 5, 2024 12:20
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 5, 2024
Fixes llvm#105813 and
llvm#106915.
Adds a check for the end of the iterator, which can be a sentinel.
The issue was introduced in
llvm@0efe111
from what I can see, so along with the introduction of /EHa support.

(cherry picked from commit 2e0ded3)
@mstorsjo
Copy link
Member

@R-Goc See https://github.com/llvm/llvm-project/blob/main/.github/workflows/email-check.yaml#L29-L39, https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it and https://github.com/settings/emails. (There's an automation workflow that's supposed to detect this and remind you about it, but it seems to not have run here.)

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 13, 2024
Fixes llvm#105813 and
llvm#106915.
Adds a check for the end of the iterator, which can be a sentinel.
The issue was introduced in
llvm@0efe111
from what I can see, so along with the introduction of /EHa support.

(cherry picked from commit 2e0ded3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-cl.exe 19.1.0 and 20.0.0 hangs non-deterministically
5 participants