This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Generate fixups as emitting DWARF .debug_frame/.eh_frame.
ClosedPublic

Authored by HsiangKai on Feb 17 2019, 3:45 PM.

Details

Summary

It is necessary to generate fixups in .debug_frame or .eh_frame as
relaxation is enabled due to the address delta may be changed after
linking.

There is an opcode with 6-bits data in debug frame encoding. So, we
also need 6-bits fixup types.

Diff Detail

Repository
rL LLVM

Event Timeline

HsiangKai created this revision.Feb 17 2019, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2019, 3:45 PM

Perhaps we should keep getKindForSize as being in bytes, and add a new getKindForSizeInBits? This would reduce the diff, but more importantly ensures that downstream forks don't silently break until runtime due to the changed semantics of getKindForSize despite having the same type as before.

jrtc27 added inline comments.Feb 18 2019, 11:14 AM
include/llvm/MC/MCFixup.h
162 ↗(On Diff #187179)

Keep this on one line like the surroundings?

176 ↗(On Diff #187179)

Ditto.

HsiangKai marked an inline comment as done.Mar 1 2019, 10:17 PM
HsiangKai added inline comments.
include/llvm/MC/MCFixup.h
162 ↗(On Diff #187179)

Formatted by clang-format. However, I agree with you. It should be kept the same format as previous lines.

HsiangKai updated this revision to Diff 189031.Mar 1 2019, 10:25 PM
HsiangKai updated this revision to Diff 189032.Mar 1 2019, 10:29 PM

Ping. The patch is similar to D46850.

HsiangKai updated this revision to Diff 202409.May 31 2019, 3:38 AM

Support .debug_frame/.eh_frame parsing for llvm-dwarfdump.

asb added a comment.Jun 18 2019, 7:49 AM

This seems sensible to me from a RISC-V perspective, though I'd appreciate it if one of the debug info people could feedback on the DWARF related changes. Might you have a chance to look at this @aprantl @probinson @JDevlieghere?

The dwarfdump changes look okay, but the new tests don't exercise those changes. The simplest thing is probably to RUN llvm-dwarfdump in relax-debug-frame.ll and verify the output describes the frame as expected.

lib/MC/MCDwarf.cpp
1907 ↗(On Diff #202409)

bool WithFixups = (Offset != nullptr && Size != nullptr);
or even just (Offset && Size)

test/DebugInfo/RISCV/relax-debug-frame.ll
8 ↗(On Diff #202409)

Do you have a way to prove these relocations are in the right section? Right now the test verifies that they come after the start of the .rela.*_frame section, but it does not verify they come before the end of the section.

HsiangKai updated this revision to Diff 206128.Jun 22 2019, 6:33 PM

Update test cases.

HsiangKai marked an inline comment as done.Jun 22 2019, 6:43 PM

Use llvm-dwarfdump to ensure DWARF parsing is correct under relaxation.

test/DebugInfo/RISCV/relax-debug-frame.ll
8 ↗(On Diff #202409)

I check the end of section '}' before every pair of checking strings. It could ensure these strings within one section.

asb accepted this revision.Jul 15 2019, 10:10 PM

LGTM (two minor comments in-line). Pleas go ahead and commit - thanks!

include/llvm/MC/MCFixup.h
147 ↗(On Diff #206128)

isPCRel -> IsPCRel

153 ↗(On Diff #206128)

Can change if(foo)+llvm_unreachable to an assert

This revision is now accepted and ready to land.Jul 15 2019, 10:10 PM
asb requested changes to this revision.Jul 18 2019, 3:59 AM

Looking at this in more detail - shouldn't we be making use of the R_RISCV_SET* relocations?

This revision now requires changes to proceed.Jul 18 2019, 3:59 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Jul 18 2019, 7:47 AM
This revision was automatically updated to reflect the committed changes.