This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rastogishubham on Dec 8 2022, 12:23 PM.

Details

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.

Diff Detail

Event Timeline

rastogishubham created this revision.Dec 8 2022, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 12:23 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
rastogishubham requested review of this revision.Dec 8 2022, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 12:23 PM

Add a newline to the end of the test file

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

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?

@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

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 accepted this revision.Dec 12 2022, 11:35 AM
This revision is now accepted and ready to land.Dec 12 2022, 11:35 AM

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

This revision was landed with ongoing or failed builds.Dec 14 2022, 4:36 PM
This revision was automatically updated to reflect the committed changes.
rastogishubham marked an inline comment as done.

Moved test to Aarch64 folder because of test failurers

aprantl added inline comments.Dec 15 2022, 10:00 AM
llvm/test/DebugInfo/AArch64/debugframeinfo.s
1 ↗(On Diff #483040)

Sorry, I should have caught this during the review!

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?).

@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 edited the summary of this revision. (Show Details)

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.

Updated test to make sure eh_frame section is empty

aprantl added inline comments.Apr 10 2023, 3:36 PM
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?