[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

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.

[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

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]

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

[sgraenitz]

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp

387

Not relevant any more.

479

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

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

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

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/DWARFDebugLine.cpp

bool DWARFDebugLine::ParsePrologue(const DWARFDataExtractor &debug_line_data,
                                    Prologue *prologue) {
   const lldb::offset_t prologue_offset = *offset_ptr;
 
   // DEBUG_PRINTF("0x%8.8x: ParsePrologue()\n", *offset_ptr);
 
   prologue->Clear();
   uint32_t i;
   const char *s;
   prologue->total_length = debug_line_data.GetDWARFInitialLength(offset_ptr);
   prologue->version = debug_line_data.GetU16(offset_ptr);
   if (prologue->version < 2 || prologue->version > 4)
     return false;
 
   prologue->prologue_length = debug_line_data.GetDWARFOffset(offset_ptr);
   const lldb::offset_t end_prologue_offset =
       prologue->prologue_length + *offset_ptr;
   prologue->min_inst_length = debug_line_data.GetU8(offset_ptr);
bool DWARFDebugLine::ParsePrologue(const DWARFDataExtractor &debug_line_data,
   }
   return end_prologue_offset;
 }
 
 bool DWARFDebugLine::ParseSupportFiles(
     const lldb::ModuleSP &module_sp, const DWARFDataExtractor &debug_line_data,
     const lldb_private::FileSpec &cu_comp_dir, dw_offset_t stmt_list,
     FileSpecList &support_files) {
+  lldb::offset_t data_end = debug_line_data.GetByteSize();
   lldb::offset_t offset = stmt_list;
 
+  // This loop runs once for regular compile units and may run multiple times
+  // for LTO optimized ones.
+  do {
     Prologue prologue;
+    lldb::offset_t prologue_start = offset;
+
     if (!ParsePrologue(debug_line_data, &offset, &prologue)) {
       Host::SystemLog(Host::eSystemLogError, "error: parsing line table prologue "
                                              "at 0x%8.8x (parsing ended around "
                                              "0x%8.8" PRIx64 "\n",
                       stmt_list, offset);
       return false;
     }
 
     FileSpec file_spec;
     std::string remapped_file;
+    bool deduplicate = (prologue_start != stmt_list);
 
+    // File index is only used for fetching information and not stored in the
+    // file spec entries. So we don't need to unify it over multiple prologues
+    // of the compile unit.
     for (uint32_t file_idx = 1;
          prologue.GetFile(file_idx, cu_comp_dir, file_spec); ++file_idx) {
       if (module_sp->RemapSourceFile(file_spec.GetPath(), remapped_file))
         file_spec.SetFile(remapped_file, false, FileSpec::Style::native);
+
+      // Avoid adding duplicate file specs from subsequent prologues.
+      if (deduplicate)
+        support_files.AppendIfUnique(file_spec);
+      else
         support_files.Append(file_spec);
     }
+
+    // Only parse the prologue and skip the statement table.
+    // The prologue's total_length does not include the length field itself.
+    offset = prologue_start + prologue.total_length +
+             debug_line_data.GetDWARFSizeofInitialLength();
+  } while (offset < data_end);
+
   return true;
 }
 
 //----------------------------------------------------------------------
 // ParseStatementTable
 //
 // Parse a single line table (prologue and all rows) and call the callback
 // function once for the prologue (row in state will be zero) and each time a

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

bool SymbolFileDWARF::ParseImportedModules(
   }
   return false;
 }
 
 struct ParseDWARFLineTableCallbackInfo {
   LineTable *line_table;
   std::unique_ptr<LineSequence> sequence_ap;
   lldb::addr_t addr_mask;
+  const FileSpecList &support_files;
 };
 
 //----------------------------------------------------------------------
 // ParseStatementTableCallback
 //----------------------------------------------------------------------
 static void ParseDWARFLineTableCallback(dw_offset_t offset,
                                         const DWARFDebugLine::State &state,
                                         void *userData) {
   if (state.row == DWARFDebugLine::State::StartParsingLineTable) {
     // Just started parsing the line table
   } else if (state.row == DWARFDebugLine::State::DoneParsingLineTable) {
     // Done parsing line table, nothing to do for the cleanup
   } else {
     ParseDWARFLineTableCallbackInfo *info =
         (ParseDWARFLineTableCallbackInfo *)userData;
     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)
+        file_idx = promoted_idx;
+    }
+
     line_table->AppendLineEntryToSequence(
         info->sequence_ap.get(), state.address & info->addr_mask, state.line,
-        state.column, state.file, state.is_stmt, state.basic_block,
+        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();
     }
   }
 }
 
 bool SymbolFileDWARF::ParseCompileUnitLineTable(const SymbolContext &sc) {
   assert(sc.comp_unit);
   if (sc.comp_unit->GetLineTable() != NULL)
     return true;
 
   DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit);
   if (dwarf_cu) {
     const DWARFBaseDIE dwarf_cu_die = dwarf_cu->GetUnitDIEOnly();
     if (dwarf_cu_die) {
       const dw_offset_t cu_line_offset =
           dwarf_cu_die.GetAttributeValueAsUnsigned(DW_AT_stmt_list,
                                                    DW_INVALID_OFFSET);
       if (cu_line_offset != DW_INVALID_OFFSET) {
         std::unique_ptr<LineTable> line_table_ap(new LineTable(sc.comp_unit));
         if (line_table_ap.get()) {
-          ParseDWARFLineTableCallbackInfo info;
-          info.line_table = line_table_ap.get();
+          const FileSpecList &support_files = sc.comp_unit->GetSupportFiles();
+          ParseDWARFLineTableCallbackInfo info{line_table_ap.get(), nullptr, 0,
+                                               support_files};
 
           /*
            * MIPS:
            * The SymbolContext may not have a valid target, thus we may not be
            * able
            * to call Address::GetOpcodeLoadAddress() which would clear the bit
            * #0
            * for MIPS. Use ArchSpec to clear the bit #0.
           */
           ArchSpec arch;
           GetObjectFile()->GetArchitecture(arch);
           switch (arch.GetMachine()) {
           case llvm::Triple::mips:
           case llvm::Triple::mipsel:
           case llvm::Triple::mips64:
           case llvm::Triple::mips64el:
             info.addr_mask = ~((lldb::addr_t)1);
             break;
           default:
             info.addr_mask = ~((lldb::addr_t)0);
             break;
           }
 
+          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;
-          DWARFDebugLine::ParseStatementTable(get_debug_line_data(), &offset,
-                                              ParseDWARFLineTableCallback,
-                                              &info);
+
+          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()));