This is an archive of the discontinued LLVM Phabricator instance.

Use dwarfdump's frame parser instead of writing a new one in llvm-obdjump
ClosedPublic

Authored by pete on Dec 15 2015, 10:35 AM.

Details

Reviewers
rafael
Summary

Hi Rafael

Turns out all the code I wrote in llvm-objdump to handle __eh_frame entries wasn't needed. As you pointed out, we should handle the --debug=frames option in llvm-objdump and that led me to the code which can already parse CIEs and FDEs.

This patch deletes printMachOEHFrameSection from MachODump.cpp and instead improves DWARFDebugFrame::parse to also handle __eh_frame. It also hooks up the command line options to llvm-objdump and teaches the dwarf dumping code that it may be dumping EH data so should look at the .eh_frame section.

I think improving DWARFDebugFrame::parse makes sense as we already have single MC methods to emit both EH and DWARF frames (FrameEmitterImpl::EmitCIE, FrameEmitterImpl::EmitFDE), so parsing both EH and DWARF frames is analogous to this.

Diff Detail

Event Timeline

pete updated this revision to Diff 42876.Dec 15 2015, 10:35 AM
pete retitled this revision from to Use dwarfdump's frame parser instead of writing a new one in llvm-obdjump.
pete updated this object.
pete added a reviewer: rafael.
pete set the repository for this revision to rL LLVM.
pete added subscribers: lhames, enderby.
rafael edited edge metadata.Dec 16 2015, 4:05 PM

General direction looks good. Just have some comments.

include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
22

The whitespace change looks ood.

lib/DebugInfo/DWARF/DWARFContext.cpp
371

This comment is a copy and paste and is probably out of place for .eh_frame. It is based on debug_frame.

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
542

Looks like EHData is never used.
Also, where is "eh" from? I don't see it in

https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html

test/tools/llvm-objdump/eh_frame-arm64.test
1

Can you update the test to use the new command line option and delete the old one?

emaste added a subscriber: emaste.Dec 16 2015, 4:25 PM
pete added a comment.Dec 16 2015, 5:35 PM

Thanks for the review. Will get you an updated version shortly.

include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
22

Good catch. It was automatically indented. Will fix.

lib/DebugInfo/DWARF/DWARFContext.cpp
371

Ah yeah, i'll remove the bad comment.

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
542

I'd been using this site. But yeah, no idea where 'eh' comes from. We don't generate it in LLVM so i'll remove it from this code.

https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-PDA/LSB-PDA/ehframechpt.html

test/tools/llvm-objdump/eh_frame-arm64.test
1

-unwind-info is still used for other stuff. But I will remove .eh_frame dumping from that method and make this test use -debug=frames.

If we want to remove -unwind-info completely then i'm happy to, but will do in a follow up patch.

Can you update the test to use the new command line option and delete the old one?

-unwind-info is still used for other stuff. But I will remove .eh_frame dumping from that method and make this test use -debug=frames.

If we want to remove -unwind-info completely then i'm happy to, but will do in a follow up patch.

Agreed.

Thanks,
Rafael

pete updated this revision to Diff 43093.Dec 16 2015, 5:42 PM
pete edited edge metadata.
pete removed rL LLVM as the repository for this revision.

Updated to address Rafael's comment.

Thanks,
Pete

rafael added inline comments.Dec 17 2015, 6:42 AM
include/llvm/DebugInfo/DWARF/DWARFContext.h
51

Just EHFrame maybe?

174

getEHFrame

199

getEHFrameSection

251

EHFrameSection

291

getEHFrameSection

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
540

Will this ever return null? In the format it is always present. I can be empty.

564

You can use a range loop, no?

626

'z' has to be the first character.

635

Instead of checking the string again, can't you check the variables you set when parsing? If LSDAPointerEncoding has a value for this case for example.

pete added a comment.Dec 17 2015, 9:20 AM

Thanks for the comments.

I've renamed everything to just EHFrame. Will work on restructuring the loop now.

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
540

The link I gave before suggests its optional. LLVM itself will always generate at least "zR", but I have no idea about other tools.

The line in particular I think makes it optional is "A 'z' may be present as the first character of the string"

626

Good catch. I'll be changing the structure of the loop so that it starts at index 0 and makes sure that 'z' is only at index 0. That should fix this.

635

Will do.

pete updated this revision to Diff 43149.Dec 17 2015, 9:33 AM

Updated naming, restructured loop, and fixed FDE parsing to not recheck the contents of the CIE augmentation string.

rafael accepted this revision.Dec 18 2015, 8:17 AM
rafael edited edge metadata.

LGTM with a nit.

tools/llvm-objdump/llvm-objdump.cpp
1629

Shouldn't this be checking DIDT_Frames?

This revision is now accepted and ready to land.Dec 18 2015, 8:17 AM
pete added inline comments.Dec 18 2015, 9:32 AM
tools/llvm-objdump/llvm-objdump.cpp
1629

Good question. Wasn't sure exactly how we want to handle this.

The cl::opt I added was copied from llvm-dwarfdump so supports much more than just frames. If we only want llvm-objdump to support frames for now then I'll remove the other options from llvm-objdump until we want to add them later.

The objdump man page lists the following, which is why I thought we could just allow pretty much anything llvm-dwarfdump has in it, but i'm happy to restrict it to frames for now:

man page: "--dwarf[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,=frames-interp,=str,=loc,=Ranges]"

pete closed this revision.Dec 18 2015, 10:56 AM

Committed in r256008.

I removed all the other dump dwarf dump options so we just have dwarf=frames. We can add them back in as needed.

I left the conditional checks against "!= DIDT_Null" as that way we don't have to change those lines in future when we add more options to become more like objdump.

If any of that isn't quite what you were thinking, let me know and i'll be happy to change it.

Thanks again for the review, Rafael,
Pete