This is an archive of the discontinued LLVM Phabricator instance.

[dwarfdump] Make .eh_frame an alias for .debug_frame
ClosedPublic

Authored by JDevlieghere on Sep 14 2017, 8:19 AM.

Details

Summary

This patch makes the .eh_frame extension an alias for .debug_frame.
Up till now it was only possible to dump the section using objdump, but
not with dwarfdump. Since the two are essentially interchangeable, we
dump whichever of the two is present.

As a workaround, this patch also adds parsing for 3 currently
unimplemented CFA instructions: DW_CFA_def_cfa_expression,
DW_CFA_expression, and DW_CFA_val_expression. Because I lack the
required knowledge, I just parse the fields without actually creating
the instructions.

Finally, this also fixes the typo in the .debug_frame section name
which incorrectly contained a trailing s.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Sep 14 2017, 8:19 AM
rnk added inline comments.Sep 14 2017, 11:20 AM
lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
191–200 ↗(On Diff #115223)

Why does this need to be part of this change? It seems nicer to commit this without the change, XFAIL or workaround whatever test relies on these, and implement the DW_OP expression machine in a follow-up change.

JDevlieghere added inline comments.Sep 14 2017, 12:47 PM
lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
191–200 ↗(On Diff #115223)

I agree, and essentially this is the workaround. Unless we just want to bail and don't care about any subsequent instructions that we might understand, we need to at least increment the offset. Failing isn't really an option imho because .eh_frame is part of "all" and that might have a pretty big impact.

Maybe I can update the comment to stress that this is a workaround?

aprantl accepted this revision.Sep 14 2017, 1:11 PM
aprantl added inline comments.
include/llvm/DebugInfo/DIContext.h
160 ↗(On Diff #115223)

Do we want to retain the ability to verify only part of a file? E.g., --verify --debug-info?

This revision is now accepted and ready to land.Sep 14 2017, 1:11 PM
aprantl requested changes to this revision.Sep 14 2017, 1:29 PM

Sorry, I accidentally marked this as accepted.

This revision now requires changes to proceed.Sep 14 2017, 1:29 PM
aprantl added inline comments.Sep 14 2017, 1:30 PM
include/llvm/DebugInfo/DIContext.h
160 ↗(On Diff #115223)

To answer my rhetoric question: I think we do.

My impression is that .eh_frame and .debug_frame are interchangeable and in fact you get only one or the other in any given compilation. So, if you ask for .debug_frame and the object has .eh_frame, I think we should still dump it; and vice versa.

JDevlieghere added inline comments.Sep 14 2017, 2:43 PM
include/llvm/DebugInfo/DIContext.h
160 ↗(On Diff #115223)

This doesn't change the behavior though? Both the (explicit) DumpType argument and the member in DIDumpOptions are always set to the same value.

My impression is that .eh_frame and .debug_frame are interchangeable and in fact you get only one or the other in any given compilation. So, if you ask for .debug_frame and the object has .eh_frame, I think we should still dump it; and vice versa.

I think we can apply the same rules as for .dwo sections here. If DumpDebugFrame is requested we dump .debug_frame and .eh_frame, whichever exists.
For compatibility with Darwin dwarfdump we can add a --eh-frame option that is an alias to --debug-frame.

JDevlieghere edited edge metadata.
JDevlieghere retitled this revision from [dwarfdump] Make .eh_frame a first class citizen to [dwarfdump] Make .eh_frame an alias for .debug_frame.
JDevlieghere edited the summary of this revision. (Show Details)

Thanks! I've updated the diff and description to make .eh_frame an alias for .debug_frame.

aprantl added inline comments.Sep 14 2017, 4:33 PM
include/llvm/DebugInfo/DIContext.h
160 ↗(On Diff #115223)

Oh sorry, I overlooked that DumpType is a member of DumpOpts!

tools/llvm-dwarfdump/llvm-dwarfdump.cpp
64 ↗(On Diff #115303)

You could just declare it as alias(DumpDebuFrame) instead.

tools/llvm-objdump/llvm-objdump.cpp
195 ↗(On Diff #115303)

I think I would feel more comfortable if we would leave the llvm-objdump options untouched.

JDevlieghere set the repository for this revision to rL LLVM.

Feedback Adrian.

JDevlieghere marked 2 inline comments as done.Sep 15 2017, 2:04 AM
aprantl accepted this revision.Sep 15 2017, 8:06 AM
This revision is now accepted and ready to land.Sep 15 2017, 8:06 AM
This revision was automatically updated to reflect the committed changes.