DWARFContext: Make loading of sections thread-safe

[labath]

Summary

SymbolFileDWARF used to load debug sections in a thread-safe manner.
When we moved to DWARFContext, we dropped the thread-safe part, because
we thought it was not necessary.

It turns out this was only mostly correct.

The "mostly" part is there because this is a problem only if we use the
manual index, as that is the only source of intra-module paralelism.
Also, this only seems to occur for extremely simple files (like the ones
I've been creating for tests lately), where we've managed to start
indexing before loading the debug_str section. Then, two threads start
to load the section simultaneously and produce wrong results.

On more complex files, something seems to be loading the debug_str section
before we start indexing, as I haven't been able to reproduce this
there, but I have not investigated what it is.

I've tried to come up with a test for this, but I haven't been able to
reproduce the problem reliably. Still, while doing so, I created a way
to generate many compile units on demand. Given that most of our tests
work with only one or two compile units, it seems like this could be
useful anyway.

[labath]

Two other options I see are:

• initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.
• have the manual index preload the sections it needs. it already does a bunch of preloading in order to speed up the access to everything, so this wouldn't look completely out of place there.

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFContext.h

25

is llvm::once_flag better than std::once_flag?

[clayborg]

In D62316#1513894, @labath wrote:

Two other options I see are:

• initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.

I'd be fine with this.

• have the manual index preload the sections it needs. it already does a bunch of preloading in order to speed up the access to everything, so this wouldn't look completely out of place there.

I like either your current solution or the load all on creation better that this,

[labath]

In D62316#1513905, @clayborg wrote:

In D62316#1513894, @labath wrote:

Two other options I see are:

• initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.

I'd be fine with this.

Ok, so let's go with the current solution to restore status quo, and I'll return to this idea later.

source/Plugins/SymbolFile/DWARF/DWARFContext.h

25

Not really, but it's needed because std::once_flag does not work on some more exotic platforms. Elsewhere, it's equivalent to std::once_flag.

[labath]

In D62316#1515266, @labath wrote:

In D62316#1513905, @clayborg wrote:

In D62316#1513894, @labath wrote:

Two other options I see are:

• initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.

I'd be fine with this.

Ok, so let's go with the current solution to restore status quo, and I'll return to this idea later.

I've realized that this may negatively affect the modules which are being read from process memory (this does not really work for ELF or PECOFF, but MachO implements it, and loads sections lazily). Given that it also seems to be possible to create the llvm DWARFContext with a custom implementation of DWARFObject (which could probably implement lazy loading, if needed), this does not seem to be that important right now. So, I've decided to shelve the idea for the time being.

lit/SymbolFile/DWARF/parallel-indexing-stress.s

+# Stress-test the parallel indexing of compile units.
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -o %t -filetype=obj
+# RUN: %lldb %t -o "target variable A" -b | FileCheck %s
+
+# CHECK-COUNT-256: A = 47
+
+  .section  .debug_str,"MS",@progbits,1
+.Linfo_string0:
+  .asciz  "Hand-written DWARF"
+.Lname:
+  .asciz  "A"
+.Linfo_string4:
+  .asciz  "int"                   # string offset=95
+
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 37                      # DW_AT_producer
+  .byte 14                      # DW_FORM_strp
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 2                       # Abbreviation Code
+  .byte 52                      # DW_TAG_variable
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 2                       # DW_AT_location
+  .byte 24                      # DW_FORM_exprloc
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 3                       # Abbreviation Code
+  .byte 36                      # DW_TAG_base_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .byte 62                      # DW_AT_encoding
+  .byte 11                      # DW_FORM_data1
+  .byte 11                      # DW_AT_byte_size
+  .byte 11                      # DW_FORM_data1
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+
+.macro generate_unit
+  .data
+A\@:
+  .long 47
+
+  .section  .debug_str,"MS",@progbits,1
+
+  .section  .debug_info,"",@progbits
+.Lcu_begin\@:
+  .long .Ldebug_info_end\@-.Ldebug_info_start\@ # Length of Unit
+.Ldebug_info_start\@:
+  .short  4                       # DWARF version number
+  .long .debug_abbrev           # Offset Into Abbrev. Section
+  .byte 8                       # Address Size (in bytes)
+  .byte 1                       # Abbrev [1] 0xb:0x30 DW_TAG_compile_unit
+  .long .Linfo_string0          # DW_AT_producer
+  .byte 2                       # Abbrev [2] 0x1e:0x15 DW_TAG_variable
+  .long .Lname                  # DW_AT_name
+  .long .Ltype\@-.Lcu_begin\@   # DW_AT_type
+  .byte 9                       # DW_AT_location
+  .byte 3
+  .quad A\@
+.Ltype\@:
+  .byte 3                       # Abbrev [3] 0x33:0x7 DW_TAG_base_type
+  .long .Linfo_string4          # DW_AT_name
+  .byte 5                       # DW_AT_encoding
+  .byte 4                       # DW_AT_byte_size
+  .byte 0                       # End Of Children Mark
+.Ldebug_info_end\@:
+
+.endm
+
+.rept 256
+generate_unit
+.endr

source/Plugins/SymbolFile/DWARF/DWARFContext.h

 //===-- DWARFContext.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_PLUGINS_SYMBOLFILE_DWARF_DWARFCONTEXT_H
 #define LLDB_PLUGINS_SYMBOLFILE_DWARF_DWARFCONTEXT_H
 
 #include "DWARFDataExtractor.h"
 #include "lldb/Core/Section.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/Support/Threading.h"
 #include <memory>
 
 namespace lldb_private {
 class DWARFContext {
 private:
   SectionList *m_main_section_list;
   SectionList *m_dwo_section_list;
 
-  llvm::Optional<DWARFDataExtractor> m_data_debug_abbrev;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_addr;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_aranges;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_info;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_line;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_line_str;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_macro;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_str;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_str_offsets;
-  llvm::Optional<DWARFDataExtractor> m_data_debug_types;
+  struct SectionData {
+    llvm::once_flag flag;
+    DWARFDataExtractor data;
+  };
+
+  SectionData m_data_debug_abbrev;
+  SectionData m_data_debug_addr;
+  SectionData m_data_debug_aranges;
+  SectionData m_data_debug_info;
+  SectionData m_data_debug_line;
+  SectionData m_data_debug_line_str;
+  SectionData m_data_debug_macro;
+  SectionData m_data_debug_str;
+  SectionData m_data_debug_str_offsets;
+  SectionData m_data_debug_types;
 
   bool isDwo() { return m_dwo_section_list != nullptr; }
 
+  const DWARFDataExtractor &
+  LoadOrGetSection(lldb::SectionType main_section_type,
+                   llvm::Optional<lldb::SectionType> dwo_section_type,
+                   SectionData &data);
+
 public:
   explicit DWARFContext(SectionList *main_section_list,
                         SectionList *dwo_section_list)
       : m_main_section_list(main_section_list),
         m_dwo_section_list(dwo_section_list) {}
 
   const DWARFDataExtractor &getOrLoadAbbrevData();
   const DWARFDataExtractor &getOrLoadAddrData();

source/Plugins/SymbolFile/DWARF/DWARFContext.cpp

static DWARFDataExtractor LoadSection(SectionList *section_list,
   if (!section_sp)
     return DWARFDataExtractor();
 
   DWARFDataExtractor data;
   section_sp->GetSectionData(data);
   return data;
 }
 
-static const DWARFDataExtractor &
-LoadOrGetSection(SectionList *section_list, SectionType section_type,
-                 llvm::Optional<DWARFDataExtractor> &extractor) {
-  if (!extractor)
-    extractor = LoadSection(section_list, section_type);
-  return *extractor;
+const DWARFDataExtractor &
+DWARFContext::LoadOrGetSection(SectionType main_section_type,
+                               llvm::Optional<SectionType> dwo_section_type,
+                               SectionData &data) {
+  llvm::call_once(data.flag, [&] {
+    if (dwo_section_type && isDwo())
+      data.data = LoadSection(m_dwo_section_list, *dwo_section_type);
+    else
+      data.data = LoadSection(m_main_section_list, main_section_type);
+  });
+  return data.data;
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadAbbrevData() {
-  if (isDwo())
-    return LoadOrGetSection(m_dwo_section_list, eSectionTypeDWARFDebugAbbrevDwo,
-                            m_data_debug_abbrev);
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugAbbrev,
-                          m_data_debug_abbrev);
+  return LoadOrGetSection(eSectionTypeDWARFDebugAbbrev,
+                          eSectionTypeDWARFDebugAbbrevDwo, m_data_debug_abbrev);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadArangesData() {
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugAranges,
+  return LoadOrGetSection(eSectionTypeDWARFDebugAranges, llvm::None,
                           m_data_debug_aranges);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadAddrData() {
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugAddr,
+  return LoadOrGetSection(eSectionTypeDWARFDebugAddr, llvm::None,
                           m_data_debug_addr);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadDebugInfoData() {
-  if (isDwo())
-    return LoadOrGetSection(m_dwo_section_list, eSectionTypeDWARFDebugInfoDwo,
-                            m_data_debug_info);
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugInfo,
-                          m_data_debug_info);
+  return LoadOrGetSection(eSectionTypeDWARFDebugInfo,
+                          eSectionTypeDWARFDebugInfoDwo, m_data_debug_info);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadLineData() {
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugLine,
+  return LoadOrGetSection(eSectionTypeDWARFDebugLine, llvm::None,
                           m_data_debug_line);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadLineStrData() {
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugLineStr,
+  return LoadOrGetSection(eSectionTypeDWARFDebugLineStr, llvm::None,
                           m_data_debug_line_str);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadMacroData() {
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugMacro,
+  return LoadOrGetSection(eSectionTypeDWARFDebugMacro, llvm::None,
                           m_data_debug_macro);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadStrData() {
-  if (isDwo())
-    return LoadOrGetSection(m_dwo_section_list, eSectionTypeDWARFDebugStrDwo,
-                            m_data_debug_str);
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugStr,
-                          m_data_debug_str);
+  return LoadOrGetSection(eSectionTypeDWARFDebugStr,
+                          eSectionTypeDWARFDebugStrDwo, m_data_debug_str);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadStrOffsetsData() {
-  if (isDwo())
-    return LoadOrGetSection(m_dwo_section_list, eSectionTypeDWARFDebugStrOffsetsDwo,
-                            m_data_debug_str_offsets);
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugStrOffsets,
+  return LoadOrGetSection(eSectionTypeDWARFDebugStrOffsets,
+                          eSectionTypeDWARFDebugStrOffsetsDwo,
                           m_data_debug_str_offsets);
 }
 
 const DWARFDataExtractor &DWARFContext::getOrLoadDebugTypesData() {
-  return LoadOrGetSection(m_main_section_list, eSectionTypeDWARFDebugTypes,
+  return LoadOrGetSection(eSectionTypeDWARFDebugTypes, llvm::None,
                           m_data_debug_types);
 }