This is an archive of the discontinued LLVM Phabricator instance.

[WIP] Fix parsing of OSO entries for LTO optimized compile units
AbandonedPublic

Authored by sgraenitz on Aug 31 2018, 9:52 AM.

Details

Summary

LLDB failed to resolve breakpoints in LTO optimized compile units if they come with embedded DWARF (no external dSYM).

LTO-optimized object files with embedded DWARF appear to have debug_line sections with multiple pairs of (prologue, statement table). LLDB is currently only reading the first one.

It turns out a little more tricky than expected. Will aim for a quick&conservative fix in order to make sure I found all affected spots. Then we can discuss and try to find a good implementation.

Event Timeline

sgraenitz created this revision.Aug 31 2018, 9:52 AM

This is part 1 of the fix. There seems to be a similar issue with file indexes in SymbolFileDWARFDebugMap::ParseCompileUnitLineTable().
In the meantime it would be great to find a solution for the hardcoded size of the length field, as explained in the inline comments.

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
387

The size of the total_length field is determined inside this function. I could add some code to remember it and read it in the ParseSupportFiles() function, but maybe there is a better way to tell between DWARF and DWARF64? Any ideas?

479

Need to get rid of the hardcoded size of the length field here.

sgraenitz added a project: Restricted Project.Aug 31 2018, 10:03 AM
sgraenitz added a reviewer: JDevlieghere.

I haven't looked at the patch, but you will need a test for this.

To test this it might be best to do the lto link manually using LLVM tools and then check in assembler sources for the testcase. The other option would be to check in bitcode, but I think I'd prefer something that is not binary.

[DWARF] Pass is_dwarf64 flag to ParseSupportFiles() to determine to size of the prologue length field.

sgraenitz marked an inline comment as done.Aug 31 2018, 11:20 AM

To test this it might be best to do the lto link manually using LLVM tools and then check in assembler sources for the testcase.

Ok that sounds good. I will have a look and reach out next week if I have questions.

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
387

It would be good to double-check is_dwarf64 here like this:

// DWARF64 has an additional 8 bytes length field.
assert(*offset_ptr == prologue_offset + (is_dwarf64 ? 12 : 4));

..but ParsePrologue() is called from many places.

479

Passing a is_dwarf64 flag from the caller seems the easiest solution. Is that ok?

sgraenitz updated this revision to Diff 163695.Sep 3 2018, 5:20 AM
sgraenitz marked an inline comment as done.

[DWARF] Use the data extractor's GetDWARFSizeofInitialLength() to determine to size of the prologue length field.

sgraenitz marked 2 inline comments as done.Sep 3 2018, 5:22 AM
sgraenitz added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
387

Not relevant any more.

479

Better: DWARFDataExtractor::GetDWARFSizeofInitialLength()

Can you help me understand why this patch fixes the original bug? Did we read the wrong size for the location list & how is it related to the DWARF64 flag?

sgraenitz retitled this revision from [DWARF] Fix parsing of OSO entries for LTO optimized compile units to [WIP] Fix parsing of OSO entries for LTO optimized compile units.Sep 3 2018, 8:33 AM
sgraenitz edited the summary of this revision. (Show Details)
sgraenitz marked 2 inline comments as done.Sep 3 2018, 8:50 AM

Hi Jonas, thanks for having a look. I updated the description to give a little more background.

For your question on the original bug: It was reported for a library built with monolithic LTO. When the reporter switched the debug format setting from DWARF with dSYM to DWARF, LLDB started ignoring their breakpoints.

sgraenitz updated this revision to Diff 163728.Sep 3 2018, 9:12 AM

Consider all Prologue/StatementTable pairs in SymbolFileDWARF::ParseCompileUnitLineTable() and map original file indexes to the aggregated ones in the list of support_files

sgraenitz updated this revision to Diff 163729.Sep 3 2018, 9:15 AM

Remove debug dumps

Hi all, this is all to fix the original bug, but I am sceptic it's the best way to implement it.
Added inline comments where this adds major overhead. Not sure if it can be improved easily, but I would at least try to avoid slowing down the more frequent case, when there is only one prologue&statement table in the debug_line section.

I assume dsymutil does something similar when linking debug info into one dSYM file? Maybe it's useful to have a look and streamline my approach.
@aprantl Maybe you know more about it? Where could I have a look?

Of cause I will try to get a nice test for it, but wanted to collect feedback in the meantime.
(Best view of change-set between Base and Diff4 I think.)

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
452

Trying to reduce effort here: the below AppendIfUnique() is expensive, but it greatly reduces the number of support files (in my repro it's ~150 instead of >400).

I guess dsymutil does this too?

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1036 ↗(On Diff #163728)

We read file indexes for one specific prologue (here in state.file), but what is necessary is the index in the aggregated list of support files. This does an expensive linear string search.

1099 ↗(On Diff #163728)

Same as ParseSupportFiles()

sgraenitz updated this revision to Diff 163735.Sep 3 2018, 9:57 AM

[DWARF] Squashed: Fix parsing of OSO entries for LTO optimized compile units (with aggregated debug_line sections)

clayborg requested changes to this revision.Sep 4 2018, 8:18 AM
clayborg added inline comments.
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1019–1030 ↗(On Diff #163735)

Can we put something into the userData in the ParseDWARFLineTableCallbackInfo structure that would allow us to skip this expensive check for the normal case with only one prologue? I can see this slowing down DWARF parsing a bit for a case that is rarely present. Something like:

uint32_t file_idx = state.file;
if (info->hasMultiplePrologues) {
  // code from above...
}
1021 ↗(On Diff #163735)

uint32_t file_idx = state.file;

1022–1024 ↗(On Diff #163735)

"name" might be the basename only since it might need to prepend the compile unit directory. Also, if there are path remappings this will fail here and it won't find an index and will use the wrong file index. You will need to get the full path to the file here just like in the GetSupportFiles() function:

FileSpec file_spec;
state.prologue->GetFile(file_idx, cu_comp_dir, file_spec);
if (module_sp->RemapSourceFile(file_spec.GetPath(), remapped_file))
  file_spec.SetFile(remapped_file, false, FileSpec::Style::native);
1092 ↗(On Diff #163735)

If we added a hasMultiplePrologues to ParseDWARFLineTableCallbackInfo, we can just do:

info.ParseDWARFLineTableCallbackInfo = true;

After the call to DWARFDebugLine::ParseStatementTable to let the ParseDWARFLineTableCallback know that we need to do the expensive file index lookup. Normal case would just fall through and exit the do loop after setting info.ParseDWARFLineTableCallbackInfo to true, and it then allows the slow case to do things correctly.

This revision now requires changes to proceed.Sep 4 2018, 8:18 AM
clayborg added inline comments.Sep 11 2018, 8:41 AM
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
449–464

The only way we get to a line table is by using the DW_AT_stmt_list attribute on the compile unit. How do we know it runs only once for normal compile units and multiple times for LTO? It might come down to how the line tables are padded by the linker? If somehow all compile units have line tables with no padding in between, it might trigger this loop to fail in non LTO cases? If the is the case, this change we will parse all line tables that follow the line table for the compile unit and count them as part of the current compile unit. There is no equivalent line table header like compile units have that indicates the size of the line table for the current compile unit. So if we have 3 compile units:

a.cpp
b.cpp
c.cpp

and no LTO involved, wit this change a.cpp will have all line entries for a.cpp, b.cpp and c.cpp. b.cpp will have all line entries for b.cpp and c.cpp. And c.cpp will have the correct line table.

sgraenitz abandoned this revision.May 9 2019, 2:37 AM