This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix breakpoint setting so it always works when there is a line entry in a compile unit's line table.
ClosedPublic

Authored by clayborg on Oct 18 2022, 3:10 PM.

Details

Summary

Prior to this fix, if the compile unit function:

void CompileUnit::ResolveSymbolContext(const SourceLocationSpec &src_location_spec, SymbolContextItem resolve_scope, SymbolContextList &sc_list);

was called with a resolve scope that wasn't just eSymbolContextLineEntry, we would end up calling:

line_entry.range.GetBaseAddress().CalculateSymbolContext(&sc, resolve_scope);

This is ok as long as the line entry's base address is able to be resolved back to the same information, but there were problems when it didn't. The example I found was we have a file with a bad .debug_aranges section where the address to compile unit mapping was incomplete. When this happens, the above function call to calculate the symbol context would end up matching the module and it would NULL out the compile unit and line entry, which means we would fail to set this breakpoint. We have many other clients that ask for eSymbolContextEverything as the resolve_scope, so all other locations could end up failing as well.

The solutions is to make sure the compile unit matches the current compile unit after calling the calculate symbol context. If the compile unit is NULL, then we report an error via the module/debugger as this indicates an entry in the line table fails to resolve back to any compile unit. If the compile unit is not NULL and it differs from the current compile unit, we restore the current compile unit and line entry to ensure the call to .CalculateSymbolContext doesn't match something completely different, as can easily happen if LTO or other link time optimizations are enabled that could end up outlining or merging functions.

This patch allows breakpoint succeeding to work as expected and not get short circuited by our address lookup logic failing.

Diff Detail

Event Timeline

clayborg created this revision.Oct 18 2022, 3:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 3:10 PM
clayborg requested review of this revision.Oct 18 2022, 3:10 PM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert retitled this revision from Fix breakpoint setting so it always works when there is a line entry in a compile unit's line table. to [lldb] Fix breakpoint setting so it always works when there is a line entry in a compile unit's line table..Oct 18 2022, 3:34 PM
jdoerfert removed a reviewer: jdoerfert.
jdoerfert resigned from this revision.Oct 18 2022, 3:34 PM
jingham added a subscriber: jingham.EditedOct 18 2022, 3:48 PM

Seems reasonable that when resolving the start address of a line entry from CU A we get back a line entry in CU B, then there's just something wrong in the debug info. Trying to recover whatever info is recoverable seems worthwhile: essentially by falling back to a resolve_scope of eSymbolContextLineEntry.

However, that does produce a fairly odd SymbolContext. Did we ever have a case where you can have aSymbolContext with a valid CU & LineEntry that is in no function and no block for realz? I wouldn't be altogether surprised if we had code that asks to resolve everything in ResolveSymbolContext and if we get a CU & LineEntry back, assumes you must also have a block or function, and uses them w/o checking. I can't think of a good way to defend against this except maybe saying explicitly in SymbolContext.h that because of potential bad debug info you can't make any assumptions about which entities will get filled in in a symbol context.

I also found the first of the two comments you added confusing.

lldb/source/Symbol/CompileUnit.cpp
338

This comment seems odd to me. You say "X might not be something we want to do for reason Y" right before you actually do do X.

The following comment make it seem like you more mean "The address lookup might be problematic for reasons A & B which we're going to detect in way C & fix by setting the SC by hand, which doesn't really jibe with what the first comment said.

The first comment makes me think we shouldn't be calling CalculateSymbolContext at all and do something else - it's not clear what that would be, however. So I think it is just confusing.

clayborg updated this revision to Diff 468737.Oct 18 2022, 4:24 PM

Added missing test case file and clarified comments to be more consistent.

Seems reasonable that when resolving the start address of a line entry from CU A we get back a line entry in CU B, then there's just something wrong in the debug info. Trying to recover whatever info is recoverable seems worthwhile: essentially by falling back to a resolve_scope of eSymbolContextLineEntry.

However, that does produce a fairly odd SymbolContext. Did we ever have a case where you can have aSymbolContext with a valid CU & LineEntry that is in no function and no block for realz? I wouldn't be altogether surprised if we had code that asks to resolve everything in ResolveSymbolContext and if we get a CU & LineEntry back, assumes you must also have a block or function, and uses them w/o checking. I can't think of a good way to defend against this except maybe saying explicitly in SymbolContext.h that because of potential bad debug info you can't make any assumptions about which entities will get filled in in a symbol context.

I also found the first of the two comments you added confusing.

I didn't mean to leave the first comment it actually. When I first figured out the root cause I put that in there. Let me know if the current state of the comments makes more sense

clayborg updated this revision to Diff 468738.Oct 18 2022, 4:27 PM

Added missing file for real this time.

labath added inline comments.Oct 20 2022, 4:15 AM
lldb/source/Symbol/CompileUnit.cpp
338

I think this would be more robustly written as something like

line_entry.range.GetBaseAddress().CalculateSymbolContext(&new_sc, resolve_scope);
if (new_sc.comp_unit == this)
  sc_list.Append(new_sc);
else
  sc_list.Append(sc);
  • basically using a new symbol context for the resolution, instead of twiddling the old one back and forth.
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
109

I would hope that the yaml-based test can run on windows as well.

clayborg updated this revision to Diff 469342.Oct 20 2022, 1:46 PM

Create a new "resolved_sc" that gets used to resolve the line_entry's address. If the resolving goes to plan, append that symbol context, else append the original one.

clayborg updated this revision to Diff 469344.Oct 20 2022, 1:48 PM

Also test on windows.

labath accepted this revision.Oct 24 2022, 2:03 AM
This revision is now accepted and ready to land.Oct 24 2022, 2:03 AM