Page MenuHomePhabricator

[lld][ELF] Rewrite ICF-folded references rather than tombstoning them
Needs ReviewPublic

Authored by phosek on Oct 9 2020, 12:51 AM.

Details

Summary

This modifies the logic introduced in D82828 to address the case that
was brought up in PR47150.

Currently, we use tombstone value for all discarded references in
.debug_line section, but we rewrite ICF references in other debug
sections. This introduces inconsistency between .debug_line and
.debug_info data which is a problem for consumers such as GSYM.

This change modifies the behavior to be more consistent: we always
rewrite the reference for ICF folded case across all debug sections,
and we always use the tombstone value for all other cases.

Diff Detail

Event Timeline

phosek created this revision.Oct 9 2020, 12:51 AM
phosek requested review of this revision.Oct 9 2020, 12:51 AM

In PR47150, @dblaikie suggested reopening the discussion with reviewers on D82828 which is the purpose of this change in addition to also implementing the solution proposed there.

Added two colleagues from my team who have been working on our downstream version of this and I think might have slightly better insight than me.

We decided to use a tombstone value because multiple CUs with overlapping address ranges and shared entities are problematic (PR47150#comment3).
The main reason we exclude .debug_info is because breaking points do not work on folded functions (D82828).

The patch will revert all ICF related behavior and make us go back to square one (if we do this, I don't see the value of !isDebug) and cause confusion to DWARF consumers.
Do we need this? From https://bugs.llvm.org//show_bug.cgi?id=47150#c5 this is a bug in llvm-gsymutil.

bd1976llvm added a comment.EditedOct 9 2020, 12:05 PM

I plan to investigate this further after the weekend; but, I wanted to say that the current scheme is known to be compatible with gdb and with Sony's proprietary debugger and yields a reasonable, if not ideal, debugging experience.

What testing has been performed for consumers with this proposed patch?

Does anyone have examples of common DWARF consumers producing CUs with overlapping ranges? Might help inform this feature/implementation choice.

We decided to use a tombstone value because multiple CUs with overlapping address ranges and shared entities are problematic (PR47150#comment3).
The main reason we exclude .debug_info is because breaking points do not work on folded functions (D82828).

Though in doing so we created another inconsistency - where instructions described in debug_line overlap, but aren't covered by the debug_info CU's address range description (DW_AT_low/high_pc or DW_AT_ranges).

The patch will revert all ICF related behavior and make us go back to square one (if we do this, I don't see the value of !isDebug) and cause confusion to DWARF consumers.
Do we need this? From https://bugs.llvm.org//show_bug.cgi?id=47150#c5 this is a bug in llvm-gsymutil.

Sounds like @phosek is looking to address things on a few different fronts - trying to make the DWARF more consistent (though not without other problems of more significantly overlapping CUs) while also pursuing an llvm-gsymutil improvement as well.

grimar added a subscriber: grimar.Oct 12 2020, 12:59 AM

Apologies that I haven't had much time to look into this. I tried this style of patching with Sony's proprietary debugger and the user experience w.r.t. source locations and setting breakpoints was degraded. When I find time I will try to determine what is theoretically best here and whether the degradation in the debugging experience that I saw is an artefact of our proprietary debuggers implementation or if it points to something important. We should try with other consumers as well.