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

[sgraenitz]

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.

[sgraenitz]

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 ↗

(On Diff #163552)

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 ↗

(On Diff #163552)

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

[friss]

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

[aprantl]

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.

[sgraenitz]

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

[sgraenitz]

In D51546#1221083, @aprantl wrote:

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 ↗

(On Diff #163552)

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 ↗

(On Diff #163552)

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

[sgraenitz]

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

[sgraenitz]

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp

387 ↗

(On Diff #163552)

Not relevant any more.

479 ↗

(On Diff #163552)

Better: DWARFDataExtractor::GetDWARFSizeofInitialLength()

[JDevlieghere]

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]

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]

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

[sgraenitz]

Remove debug dumps

[sgraenitz]

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

468 ↗

(On Diff #163728)

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

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.

1094

Same as ParseSupportFiles()

[sgraenitz]

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

[clayborg]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1019–1030

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

uint32_t file_idx = state.file;

1022–1024

"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

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.

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp

449–464 ↗

(On Diff #163735)

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.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (state.row == DWARFDebugLine::State::StartParsingLineTable) {
     LineTable *line_table = info->line_table;
 
     // If this is our first time here, we need to create a sequence container.
     if (!info->sequence_ap.get()) {
       info->sequence_ap.reset(line_table->CreateLineSequenceContainer());
       assert(info->sequence_ap.get());
     }
 
     // If this is a compile unit with multiple prologues, figure out the
     // file's index in the aggregated list of support files.
     uint16_t file_idx = state.file;
     if (file_idx > 0 && file_idx <= state.prologue->file_names.size()) {
       const char *name = state.prologue->file_names[file_idx - 1].name;
       FileSpec file_spec(name, false);
       uint32_t promoted_idx =
           info->support_files.FindFileIndex(0, file_spec, false);
 
-      if (promoted_idx != UINT32_MAX && promoted_idx < UINT16_MAX) {
-        llvm::dbgs() << "Promoted " << file_idx << " -> ";
-        llvm::dbgs() << promoted_idx << " (" << name << ")\n";
+      if (promoted_idx != UINT32_MAX && promoted_idx < UINT16_MAX)
         file_idx = promoted_idx;
-      } else {
-        llvm::dbgs() << "Unpromoted " << file_idx << " (" << name << ")\n";
-      }
     }
 
     line_table->AppendLineEntryToSequence(
         info->sequence_ap.get(), state.address & info->addr_mask, state.line,
         state.column, file_idx, state.is_stmt, state.basic_block,
         state.prologue_end, state.epilogue_begin, state.end_sequence);
     if (state.end_sequence) {
       // First, put the current sequence into the line table.
       line_table->InsertSequence(info->sequence_ap.get());
       // Then, empty it to prepare for the next sequence.
       info->sequence_ap->Clear();
     }
   }
 }
 
if (dwarf_cu_die) {
           }
 
           const DWARFDataExtractor &debug_line_data = get_debug_line_data();
           lldb::offset_t data_end = debug_line_data.GetByteSize();
           lldb::offset_t offset = cu_line_offset;
 
           do {
             DWARFDebugLine::ParseStatementTable(
                 debug_line_data, &offset, ParseDWARFLineTableCallback, &info);
           } while (offset < data_end);
 
           SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
           if (debug_map_symfile) {
             // We have an object file that has a line table with addresses that
             // are not linked. We need to link the line table and convert the
             // addresses that are relative to the .o file into addresses for
             // the main executable.
             sc.comp_unit->SetLineTable(
                 debug_map_symfile->LinkOSOLineTable(this, line_table_ap.get()));