Emit unwind information in the .debug_frame section when the .cfi_sections .debug_frame directive is used.

[rastogishubham]

Summary

By default the unwind information is emitted in the eh_frame section, but when the

.cfi_sections .debug_frame

directive is used, the information is supposed to be emitted in the .debug_frame section. This doesn't seem to work, however. This patch addresses that bug.

[rastogishubham]

Add a newline to the end of the test file

[aprantl]

Can you explain why this works?

It looks like you're effectively calling MCDwarfFrameemitter::Emit() twice with the exact same input (since it checks for IsEH || IsDebugFrame) if both flags are true?

llvm/lib/MC/MCObjectStreamer.cpp

198 ↗

(On Diff #481405)

Why introduce two flags in Emit() if one is always the inverse of the other?

llvm/test/DebugInfo/debugframeinfo.s

3

You need to restrict this testcase to some specific architecture or check less specific things. For example, there are AArch64 register names in the CHECK output.

Generally, you just want to test that it is emitted at all, so checking just that any contents exists in the section should be enough, right?

[rastogishubham]

@aprantl

My thought process was, if Emit is to be called by something else that does not set those flags to be true, then the debug frame data should not be emitted, but that should never happen, since the whole point of the function is to emit the debug frame data. I can simply do this by not checking for

isEH

[rastogishubham]

Addressed review feedback, reduced the test to not have any references to AArch63 registers and added a triple to the run line. Removed the extra IsDebugFrame bool and made the check for emission of debug frame information simpler

[aprantl]

Let's give this a try. Let's watch the bots carefully for any regressions.

[rastogishubham]

Moved test to Aarch64 folder because of test failurers

[aprantl]

llvm/test/DebugInfo/AArch64/debugframeinfo.s

1 ↗

(On Diff #483040)

Sorry, I should have caught this during the review!

[glandium]

FWIW, I'm seeing targetAtom != NULL assertions in function Fixup in ld.hpp in ld64 when build Firefox with LTO since this change (I can only guess the crashes mentioned in the revert are the same?).

[rastogishubham]

@glandium Yes you are right, those are the same issues that we saw.

llvm/test/DebugInfo/debugframeinfo.s

3

How does it look now?

[rastogishubham]

The original patch was not addressing the issue correctly. The problem is that the function

MCDwarfFrameEmitter::Emit

skips emitting the CIE and FDE in arm64 because the architecture supports emitting unwind information without having an eh frame section. This doesn't however mean that the debug frame section should be omitted if the

.cfi_sections .debug_frame

directive is used. This patch just makes sure that the CIE and FDE is emitted in the debug frame section if that is required.

[rastogishubham]

Updated test to make sure eh_frame section is empty

[aprantl]

llvm/lib/MC/MCDwarf.cpp

1858

unrelated formatting change

1878

The old condition made sense on its own, but now it may be useful to comment why we want to continue when IsEH==true?

1905

The old condition was basically: "prefer dwarf over eh_frame". What does the IsEH flag mean and should we add a comment here?

llvm/lib/MC/MCDwarf.cpp

 
 } // end namespace llvm
 
 void MCDwarfFrameEmitter::Emit(MCObjectStreamer &Streamer, MCAsmBackend *MAB,
                                bool IsEH) {
   MCContext &Context = Streamer.getContext();
   const MCObjectFileInfo *MOFI = Context.getObjectFileInfo();
   const MCAsmInfo *AsmInfo = Context.getAsmInfo();
   FrameEmitterImpl Emitter(IsEH, Streamer);
   ArrayRef<MCDwarfFrameInfo> FrameArray = Streamer.getDwarfFrameInfos();
 
   // Emit the compact unwind info if available.
   bool NeedsEHFrameSection = !MOFI->getSupportsCompactUnwindWithoutEHFrame();
-  if (IsEH && MOFI->getCompactUnwindSection()) {
+  if (MOFI->getCompactUnwindSection()) {
     Streamer.generateCompactUnwindEncodings(MAB);
     bool SectionEmitted = false;
     for (const MCDwarfFrameInfo &Frame : FrameArray) {
       if (Frame.CompactUnwindEncoding == 0) continue;
       if (!SectionEmitted) {
         Streamer.switchSection(MOFI->getCompactUnwindSection());
         Streamer.emitValueToAlignment(Align(AsmInfo->getCodePointerSize()));
         SectionEmitted = true;
       }
       NeedsEHFrameSection |=
         Frame.CompactUnwindEncoding ==
           MOFI->getCompactUnwindDwarfEHFrameOnly();
       Emitter.EmitCompactUnwind(Frame);
     }
   }
 
   if (!NeedsEHFrameSection) return;
 
   MCSection &Section =
       IsEH ? *const_cast<MCObjectFileInfo *>(MOFI)->getEHFrameSection()
            : *MOFI->getDwarfFrameSection();
 
   Streamer.switchSection(&Section);
void MCDwarfFrameEmitter::Emit(MCObjectStreamer &Streamer, MCAsmBackend *MAB,
   // an FDE refers to a CIE other than the closest previous CIE.
   std::vector<MCDwarfFrameInfo> FrameArrayX(FrameArray.begin(), FrameArray.end());
   llvm::stable_sort(FrameArrayX,
                     [](const MCDwarfFrameInfo &X, const MCDwarfFrameInfo &Y) {
                       return CIEKey(X) < CIEKey(Y);
                     });
   for (auto I = FrameArrayX.begin(), E = FrameArrayX.end(); I != E;) {
     const MCDwarfFrameInfo &Frame = *I;
     ++I;
     if (CanOmitDwarf && Frame.CompactUnwindEncoding !=
           MOFI->getCompactUnwindDwarfEHFrameOnly())
       // Don't generate an EH frame if we don't need one. I.e., it's taken care
       // of by the compact unwind encoding.
       continue;
 
     CIEKey Key(Frame);
     const MCSymbol *&CIEStart = IsEH ? CIEStarts[Key] : DummyDebugKey;

llvm/test/DebugInfo/debugframeinfo.s

+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin22.1.0 %s -o %t.o
+# RUN: llvm-dwarfdump -debug-frame %t.o | FileCheck %s
+
+# CHECK: .debug_frame contents:
+# CHECK-EMPTY:
+# CHECK-NEXT: 00000000 00000014 ffffffff CIE
+# CHECK: .eh_frame contents:
+# CHECK-EMPTY:
+
+ .cfi_startproc
+ .cfi_signal_frame
+ .cfi_def_cfa x28, 0x340
+ .cfi_endproc
+ .cfi_sections .debug_frame