Fix the nested frame case and document the updateReceiver behavior

Author: Amirhossein Pashaeehir

llvm/include/llvm/DWARFCFIChecker/DWARFCFIFunctionFrameStreamer.h

@@ -60,12 +60,27 @@ class CFIFunctionFrameStreamer : public MCStreamer {
   void emitCFIEndProcImpl(MCDwarfFrameInfo &CurFrame) override;
 
 private:
+  /// This method sends the last instruction, along with its associated
+  /// directives, to the receiver and then updates the internal state of the
+  /// class. It moves the directive index to after the last directive and sets
+  /// the last instruction to \p NewInst . This method assumes it is called in
+  /// the middle of an unfinished DWARF debug frame; if not, an assertion will
+  /// fail.
   void updateReceiver(const std::optional<MCInst> &NewInst);
 
 private:
-  std::vector<std::optional<MCInst>> FrameLastInstructions; //! FIXME
-  std::vector<unsigned> FrameLastDirectiveIndices;          //! FIXME
+  /// The following fields are stacks that store the state of the stream sent to
+  /// the receiver in each frame. This class, like `MCStreamer`, assumes that
+  /// the debug frames are intertwined with each other only in stack form.
+
+  /// The last instruction that is not sent to the receiver for each frame.
+  std::vector<std::optional<MCInst>> LastInstructions;
+  /// The index of the last directive that is not sent to the receiver for each
+  /// frame.
+  std::vector<unsigned> LastDirectiveIndices;
+  /// The index of each frame in `DwarfFrameInfos` field in `MCStreamer`.
   std::vector<unsigned> FrameIndices;
+
   std::unique_ptr<CFIFunctionFrameReceiver> Receiver;
 };
 

llvm/lib/DWARFCFIChecker/DWARFCFIFunctionFrameStreamer.cpp

@@ -19,15 +19,16 @@ using namespace llvm;
 
 void CFIFunctionFrameStreamer::updateReceiver(
     const std::optional<MCInst> &NewInst) {
-  assert(!FrameIndices.empty() && hasUnfinishedDwarfFrameInfo() &&
-         "FunctionUnitStreamer frame indices should be synced with "
-         "MCStreamer's"); //! FIXME split this assertions and also check another
-                          //! vectors
-                          //! Add tests for nested frames
+  assert(hasUnfinishedDwarfFrameInfo() &&
+         "should have an unfinished DWARF frame here");
+  assert(!FrameIndices.empty() &&
+         "there should be an index available for the current frame");
+  assert(FrameIndices.size() == LastInstructions.size());
+  assert(LastInstructions.size() == LastDirectiveIndices.size());
 
   auto Frames = getDwarfFrameInfos();
   assert(FrameIndices.back() < Frames.size());
-  unsigned LastDirectiveIndex = FrameLastDirectiveIndices.back();
+  unsigned LastDirectiveIndex = LastDirectiveIndices.back();
   unsigned CurrentDirectiveIndex =
       Frames[FrameIndices.back()].Instructions.size();
   assert(CurrentDirectiveIndex >= LastDirectiveIndex);
@@ -41,41 +42,46 @@ void CFIFunctionFrameStreamer::updateReceiver(
             .drop_back(LastFrame->Instructions.size() - CurrentDirectiveIndex);
   }
 
-  auto MaybeLastInstruction = FrameLastInstructions.back();
+  auto MaybeLastInstruction = LastInstructions.back();
   if (MaybeLastInstruction)
+    // The directives are associated with an instruction.
     Receiver->emitInstructionAndDirectives(*MaybeLastInstruction, Directives);
   else
+    // The directives are the prologue directives.
     Receiver->startFunctionFrame(false /* TODO: should put isEH here */,
                                  Directives);
 
-  FrameLastInstructions.back() = NewInst;
-  FrameLastDirectiveIndices.back() = CurrentDirectiveIndex;
+  // Update the internal state for the top frame.
+  LastInstructions.back() = NewInst;
+  LastDirectiveIndices.back() = CurrentDirectiveIndex;
 }
 
 void CFIFunctionFrameStreamer::emitInstruction(const MCInst &Inst,
                                                const MCSubtargetInfo &STI) {
-  if (hasUnfinishedDwarfFrameInfo()) {
+  if (hasUnfinishedDwarfFrameInfo())
+    // Send the last instruction with the unsent directives already in the frame
+    // to the receiver.
     updateReceiver(Inst);
-  }
 }
 
 void CFIFunctionFrameStreamer::emitCFIStartProcImpl(MCDwarfFrameInfo &Frame) {
-  FrameLastInstructions.push_back(std::nullopt);
-  FrameLastDirectiveIndices.push_back(0);
+  LastInstructions.push_back(std::nullopt);
+  LastDirectiveIndices.push_back(0);
   FrameIndices.push_back(getNumFrameInfos());
 
   MCStreamer::emitCFIStartProcImpl(Frame);
 }
 
 void CFIFunctionFrameStreamer::emitCFIEndProcImpl(MCDwarfFrameInfo &CurFrame) {
+  // Send the last instruction with the final directives of the current frame to
+  // the receiver.
   updateReceiver(std::nullopt);
 
   assert(!FrameIndices.empty() && "There should be at least one frame to pop");
-  FrameLastDirectiveIndices.pop_back();
-  FrameLastInstructions.pop_back();
+  LastDirectiveIndices.pop_back();
+  LastInstructions.pop_back();
   FrameIndices.pop_back();
 
-  dbgs() << "finishing frame\n";
   Receiver->finishFunctionFrame();
 
   MCStreamer::emitCFIEndProcImpl(CurFrame);

llvm/test/DWARFCFIChecker/X86/empty-nested-frames.s

@@ -0,0 +1,16 @@
+# RUN: llvm-mc %s --validate-cfi --filetype=null 2>&1 \
+# RUN:   | FileCheck %s --allow-empty 
+# CHECK-NOT: warning:
+
+.pushsection A
+f: 
+.cfi_startproc
+.pushsection B
+g: 
+.cfi_startproc
+ret
+.cfi_endproc
+.popsection
+ret
+.cfi_endproc
+.popsection

llvm/test/DWARFCFIChecker/X86/nested-frames.s

@@ -0,0 +1,31 @@
+# RUN: not llvm-mc %s --validate-cfi --filetype=null 2>&1 \
+# RUN:   | FileCheck %s
+
+.pushsection A
+f: 
+.cfi_startproc
+# TODO: Remove this line when the initial frame directives set the callee saved registers
+.cfi_undefined %flags
+addq $10, %rbp
+# CHECK: error: changed register RBP, that register RBP's unwinding rule uses, but there is no CFI directives about it
+nop
+.cfi_undefined %rbp
+
+.pushsection B
+g: 
+.cfi_startproc
+# TODO: Remove this line when the initial frame directives set the callee saved registers
+.cfi_undefined %flags
+addq $10, %rbp
+# CHECK: error: changed register RBP, that register RBP's unwinding rule uses, but there is no CFI directives about it
+nop
+.cfi_undefined %rsi
+ret
+.cfi_endproc
+.popsection
+
+addq $10, %rsi
+# CHECK: error: changed register RSI, that register RSI's unwinding rule uses, but there is no CFI directives about it
+ret
+.cfi_endproc
+.popsection