Page MenuHomePhabricator

[cfi] Add flag to always generate .debug_frame
Needs ReviewPublic

Authored by dcandler on Sep 5 2019, 2:22 AM.

Details

Summary

This adds a flag to LLVM and clang to always generate a .debug_frame, even if other debug information is not being generated. This would be useful for the Arm toolchain where the .debug_frame section is always expected to be present (with or without other debug sections) and can be used to calculate stack usage, although the flag itself has been left generic to cover any other potential situations where .debug_frame is specifically desired.

Diff Detail

Event Timeline

dcandler created this revision.Sep 5 2019, 2:22 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 5 2019, 2:22 AM
ostannard requested changes to this revision.Sep 5 2019, 2:56 AM
ostannard added a subscriber: ostannard.

Does the name of the -falways-need-cfi option match any existing compiler (google doesn't find anything)? If not, I think it would make more sense as a -g option, as it's similar to -gline-tables-only.

clang/lib/Frontend/CompilerInvocation.cpp
959

Is this option actually being read anywhere?

This revision now requires changes to proceed.Sep 5 2019, 2:56 AM

I was actually torn myself on whether to put the flag in the g group or not, so I'm happy to rename it. As far as I could find, no compiler has an existing option to control this: instead armcc always includes a debug_frame section by default to follow Arm's Dwarf specification. Having it as an option seems more flexible than forcing a different behavior.

clang/lib/Frontend/CompilerInvocation.cpp
959

Not any more, since I moved the option directly into AsmPrinter. I'll make sure this (and the other line) doesn't get included in the next diff.

I feel that using a -g option make more sense.

Maybe -gdwarf-frame, or -gdwarf-frame-always might be more user friendly as it relates more to the DWARF section created rather than the section content.

Does the current code generate all the prologue cfi directives?
Epilogue too? The last time I checked, it did not (especially for
epilogues).

Fully asynch prologue/epilogue is one of the things that we need for
OpenVMS and will be doing that work. We're also looking at using the
compact format instead of the CFI form (at least for routine bodies).

clang's driver already recognizes "-fasynchronous-unwind-tables" today
and maps that to "-munwind-tables" and probably should be switched to
setting your new "always-need-cfi" or simply change your name. Either
way, I think there is an interaction between them.

From the code in Clang.cpp

// This is a coarse approximation of what llvm-gcc actually does, both
// -fasynchronous-unwind-tables and -fnon-call-exceptions interact in more
// complicated ways.
dcandler updated this revision to Diff 224581.Fri, Oct 11, 6:16 AM
dcandler retitled this revision from [cfi] Add flag to always generate call frame information to [cfi] Add flag to always generate .debug_frame.
dcandler edited the summary of this revision. (Show Details)
dcandler added reviewers: rengolin, joerg.

I've modified the patch so that the new flag will ensure the cfi instructions are actually present to be emitted as well. I went ahead and renamed the flag -gdwarf-frame too, to better reflect that it's dealing with the debug information you'd otherwise get with -g, and is meant to specifically put the information in a .debug_frame section and not .eh_frame.

Currently, two things signal for need for cfi: exceptions (via the function's needsUnwindTableEntry()), and debug (via the machine module information's hasDebugInfo()). At frame lowering, both trigger the same thing. But when the assembly printer decides on which section to use, needsUnwindTableEntry() is checked first and triggers the need for .eh_frame, while hasDebugInfo() is checked afterwards for whether .debug_frame is needed. So .debug_frame is only present when any level of debug is requested, and no functions need unwinding for exceptions.

It wouldn't be appropriate to change either needsUnwindTableEntry() or hasDebugInfo(), so I've added a check for my flag alongside them. Because the same logic is used in multiple places, I've wrapped all three checks into one function to try and clean things up slightly. When deciding on which section to emit, the new flag means .debug_frame is produced instead of nothing. If .eh_frame would have been needed, rather than replace it, the new flag simply emits both .debug_frame and .eh_frame.

The end result is that -gdwarf-frame should only provide a .debug_frame section as additional information, without otherwise modifying anything. The existing -funwind-tables (and -fasynchronous-unwind-tables) flag can be used to provide similar information, but because it takes the exception angle, it alters function attributes and ultimately produces .eh_frame instead.

Ping?

I already spotted the line in CommandFlags.inc needs formatting with a couple of breaks. Also the help text in Options.td could be clearer. In particular, -gno-dwarf-frame shouldn't suggest .debug_frame won't be generated at all (since -g might still emit it). It should probably be more along the lines of:

-gdwarf-frame: "Emit a debug_frame section"
-gno-dwarf-frame: "Don't explicitly emit a debug_frame section"

I don't like the idea of adding a -gno-dwarf-frame option which doesn't always disable emission of .debug_frame. Since it doesn't make sense to emit other debug info without .debug_frame, maybe we should just not have a negative option?

I added the negative option more as a way to disable the flag, since I'm currently looking at cases where it may want to be turned on by default (and a negative option would then allow you to only get .eh_frame in cases where you'd get both .debug_frame/.eh_frame).

The other suggested name of -gdwarf-frame-always might then better describe the behavior, as the negative -gno-dwarf-frame-always wouldn't sound quite so misleading (since 'not always' equates to 'sometimes').

How about -f[no-]force-dwarf-frame, which I think matches the spelling and behaviour of the existing -fforce-enable-int128 and -fforce-emit-vtables options?