This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Use a function-local offset for AT_call_return_pc
ClosedPublic

Authored by vsk on Oct 19 2018, 6:56 PM.

Details

Summary

This is a speculative fix for tail call frame support on Linux.

Logs provided by @stella.stamenova indicate that on Linux, lldb adds a spurious slide offset to the return PC it loads from AT_call_return_pc attributes (see the list thread: "[PATCH] D50478: Add support for artificial tail call frames").

This patch side-steps the issue by getting rid of the load address calculation in lldb's CallEdge::GetReturnPCAddress.

The idea is to have the DWARF writer emit function-local offsets to the instruction after a call. I.e. return-pc = label-after-call-insn - function-entry. LLDB can simply add this offset to the base address of a function to get the return PC.

I'm open to other ideas here. I've tested that this works on Darwin, but not on Linux.

Diff Detail

Event Timeline

vsk created this revision.Oct 19 2018, 6:56 PM

The code LGTM. @stella.stamenova are you able to test the patch to verify that it actually fixes the problem?

stella.stamenova accepted this revision.Oct 22 2018, 1:40 PM

This change fixes the problem on Linux. Thanks!

This revision is now accepted and ready to land.Oct 22 2018, 1:40 PM
This revision was automatically updated to reflect the committed changes.

Am I mistaken, or is this non-conforming? The attribute has form DW_FORM_addr, but doesn't contain an address, it contains an offset relative to the start of the function?

While this conveniently avoids a relocation, it doesn't seem right. (it looks especially weird when you see DW_FORM_addr in a .dwo file where there can be no relocations, etc)

I think this would need something in the spec to support relative return_pc (perhaps the same way high_pc is spec'd? ("If the value of the DW_AT_high_pc is of class address, it is the address of the first location past the last instruction associated with the entity; if it is of class constant, the value is an unsigned integer offset which when added to the low PC gives the address of the first location past the last instruction associated with the entity" - in fact some generalization of that language could somewhat sidestep my "addr+offset" form request - if low_pc and return_pc got the language "if it's a constant, it's relative to the subprogram's low_pc" or relative to the nearest enclosing scope low_pc? (neither of those is perfectly satisfying though - with Propeller producing basic block sections, addresses within a function might not be relative to the start of the function (they might be relative to one of the floating basic block sections)... ))

(tagging the usual suspects - @aprantl @JDevlieghere @probinson @labath )

Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2019, 1:30 PM

Oh, also - no test case.

@vsk - you should probably be aware of this. I would imagine reverting this patch/restoring the spec behavior would regress object file size a fair bit by adding relocations to every call_site's return address...

(we really shouldn't have any abstraction that allows creating a DW_FORM_addr unconditionally - since that's not a valid form in Split DWARF, it should always go through something that adds the address to the address pool and uses FORM_addrx in Split DWARF at least (& probably uses addrx (or something more general like the hypothetical addr_offset, or a constant relative (relative to what, as discussed in my previous comment) encoding, etc) - which is llvm::DwarfDebug::addLabelAddress)

Yes, having a DW_FORM_addr which is actually not an address does not sound optimal. Using a data form would be better, and more consistent with the handling of DW_AT_high_pc.

About the lldb side, handling an DW_FORM_addr in a way which works for all debug info formats is tricky, but possible. IIRC, it involves resolving the address to a section+offset pair using the sections of the file which contains the debug info (so, the .o file in case of non-dsym mach-o), and then calling SymbolFileDWARF->FixupAddress(), which will translate that to the sections of the actual executable (in case of .o; otherwise it's a no-op). It shouldn't be necessary to use function-local offsets just for that, though it may be interesting to use them as an optimization for other reasons (but that won't work for propeller'd functions or functions split into hot+cold sections, etc..)

vsk added a comment.Jan 9 2020, 6:22 PM

Really sorry it's taken a while to get to this, work had really piled up over the holidays. As a first step, I've put up D72489 to make llvm's DW_AT_call_return_pc standards-compliant without regressing lldb functionality. I'm not sure how to avoid paying for a relocation without things breaking in Propeller-ed binaries, as I haven't really had the time to think about deeply. Definitely open to suggestions.