[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356612: Introduce DWARFContext. (authored by zturner, 
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59562?vs=191510&id=191572#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562

Files:
  lldb/trunk/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFContext.h
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp

Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
===
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
@@ -19,6 +19,10 @@
 #include "lldb/lldb-private.h"
 #include "llvm/Support/Error.h"
 
+namespace lldb_private {
+class DWARFContext;
+}
+
 typedef std::multimap
 CStringToDIEMap;
 typedef CStringToDIEMap::iterator CStringToDIEMapIter;
@@ -32,7 +36,7 @@
   const dw_offset_t next_offset,
   const uint32_t depth, void *userData);
 
-  DWARFDebugInfo();
+  explicit DWARFDebugInfo(lldb_private::DWARFContext &context);
   void SetDwarfData(SymbolFileDWARF *dwarf2Data);
 
   size_t GetNumCompileUnits();
@@ -62,6 +66,7 @@
   // Member variables
   //--
   SymbolFileDWARF *m_dwarf2Data;
+  lldb_private::DWARFContext &m_context;
   CompileUnitColl m_compile_units;
   std::unique_ptr
   m_cu_aranges_up; // A quick address to compile unit table
Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -355,12 +355,13 @@
   << 32), // Used by SymbolFileDWARFDebugMap to
   // when this class parses .o files to
   // contain the .o file index/ID
-  m_debug_map_module_wp(), m_debug_map_symfile(NULL), m_data_debug_abbrev(),
-  m_data_debug_aranges(), m_data_debug_frame(), m_data_debug_info(),
-  m_data_debug_line(), m_data_debug_macro(), m_data_debug_loc(),
-  m_data_debug_ranges(), m_data_debug_rnglists(), m_data_debug_str(),
-  m_data_apple_names(), m_data_apple_types(), m_data_apple_namespaces(),
-  m_abbr(), m_info(), m_line(), m_fetched_external_modules(false),
+  m_debug_map_module_wp(), m_debug_map_symfile(NULL),
+  m_context(*objfile->GetModule()), m_data_debug_abbrev(),
+  m_data_debug_frame(), m_data_debug_info(), m_data_debug_line(),
+  m_data_debug_macro(), m_data_debug_loc(), m_data_debug_ranges(),
+  m_data_debug_rnglists(), m_data_debug_str(), m_data_apple_names(),
+  m_data_apple_types(), m_data_apple_namespaces(), m_abbr(), m_info(),
+  m_line(), m_fetched_external_modules(false),
   m_supports_DW_AT_APPLE_objc_complete_type(eLazyBoolCalculate), m_ranges(),
   m_unique_ast_type_map() {}
 
@@ -394,15 +395,6 @@
 
 void SymbolFileDWARF::InitializeObject() {
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO);
-  ModuleSP module_sp(m_obj_file->GetModule());
-  if (module_sp) {
-const SectionList *section_list = module_sp->GetSectionList();
-Section *section =
-section_list->FindSectionByName(GetDWARFMachOSegmentName()).get();
-
-if (section)
-  m_obj_file->ReadSectionData(section, m_dwarf_data);
-  }
 
   if (!GetGlobalPluginProperties()->IgnoreFileIndexes()) {
 DWARFDataExtractor apple_names, apple_namespaces, apple_types, apple_objc;
@@ -549,19 +541,15 @@
   DWARFDataExtractor &data) {
   ModuleSP module_sp(m_obj_file->GetModule());
   const SectionList *section_list = module_sp->GetSectionList();
-  if (section_list) {
-SectionSP section_sp(section_list->FindSectionByType(sect_type, true));
-if (section_sp) {
-  // See if we memory mapped the DWARF segment?
-  if (m_dwarf_data.GetByteSize()) {
-data.SetData(m_dwarf_data, section_sp->GetOffset(),
- section_sp->GetFileSize());
-  } else {
-if (m_obj_file->ReadSectionData(section_sp.get(), data) == 0)
-  data.Clear();
-  }
-}
-  }
+  if (!section_list)
+return;
+
+  SectionSP section_sp(section_lis

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

The suggestion just avoids the Clear() call at the expense of a branch. No big 
deal. Fine either way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner marked an inline comment as done.
zturner added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:552
+  data.Clear();
+  m_obj_file->ReadSectionData(section_sp.get(), data);
 }

clayborg wrote:
> clayborg wrote:
> > ```
> > if (m_obj_file->ReadSectionData(section_sp.get(), data) == 0)
> >   data.Clear();
> > ```
> What do you think about this one? This is my last nit
I did see that one, but I was having trouble figuring out how the behavior is 
different from what I already wrote.  The only way I could see it being 
different is if `ReadSectionData` would actually write data there even when it 
failed.  That would be strange though, do you know if it can happen?

The way I wrote it, it clears first, and then if the function fails / returns 
0, I would expect it to be unchanged.

Happy to change it if there's actually a difference I'm overlooking, but if 
they're the same then I think the branchless version is slightly easier to read.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:552
+  data.Clear();
+  m_obj_file->ReadSectionData(section_sp.get(), data);
 }

clayborg wrote:
> ```
> if (m_obj_file->ReadSectionData(section_sp.get(), data) == 0)
>   data.Clear();
> ```
What do you think about this one? This is my last nit


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner marked 2 inline comments as done.
zturner added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:50
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, m_dwarf_data, 
eSectionTypeDWARFDebugAranges,

clayborg wrote:
> rename to "getArangesData()"? Would it be better to return an 
> Optional?
I actually prefer to keep the name as `getOrLoad`.  `get` functions often sound 
like they don't modify anything, but this way it's clear that lazy evaluation 
will occur, which might be important to the caller for some reason or another.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:24
+  // forward.
+  extractor.emplace();
+

clayborg wrote:
> clayborg wrote:
> > Don't we want to put an empty DWARFDataExtractor in here? Maybe this code 
> > should be:
> > 
> > ```
> > const SectionList *section_list = module.GetSectionList();
> >   if (!section_list)
> > return nullptr;
> > 
> >   auto section_sp = section_list->FindSectionByType(section_type, true);
> >   if (!section_sp)
> > return nullptr;
> > 
> >   extractor.emplace();
> >   if (section_sp->GetSectionData(*extractor) == 0)
> > extractor = llvm::None;
> >   return extractor.getPointer();
> > ```
> Or use a local DWARFDataExtractor and move it into "extractor" if we succeed?
> 
> ```
> DWARFDataExtractor data;
> if (section_sp->GetSectionData(data) > 0)
> extractor = std::move(data);
> ```
The suggested code here has a bug.  If you say 

```
extractor = llvm::None;
return extractor.getPointer();
```

it will assert, because there is no pointer, it's uninitialized.  When i write 
`extractor.emplace();` it initializes it with a default constructed 
`DataExtractor`, the idea being that if we try this function and it fails for 
any reason, we should not try the function again, because it already failed 
once, and so we should just "cache" the fact that it failed and immediately 
return.  By having a default constructed `DataExtractor` and changing the fast 
path exit condition to also return null in the case where the size is 0, we 
ensure that we only ever do work once, regardless of whether that work fails or 
succeeds.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, eSectionTypeDWARFDebugAranges,

clayborg wrote:
> Why do we care about "getOrLoadArangesData()"? What not just 
> "getArangesData()"?
I like to emphasize in the name that the function might do work.  People are 
trained to think of `get` functions as read-only, but it's not the case here 
and so the idea is to make sure that nobody can misinterpret the behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:24
+  // forward.
+  extractor.emplace();
+

clayborg wrote:
> Don't we want to put an empty DWARFDataExtractor in here? Maybe this code 
> should be:
> 
> ```
> const SectionList *section_list = module.GetSectionList();
>   if (!section_list)
> return nullptr;
> 
>   auto section_sp = section_list->FindSectionByType(section_type, true);
>   if (!section_sp)
> return nullptr;
> 
>   extractor.emplace();
>   if (section_sp->GetSectionData(*extractor) == 0)
> extractor = llvm::None;
>   return extractor.getPointer();
> ```
Or use a local DWARFDataExtractor and move it into "extractor" if we succeed?

```
DWARFDataExtractor data;
if (section_sp->GetSectionData(data) > 0)
extractor = std::move(data);
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:24
+  // forward.
+  extractor.emplace();
+

Don't we want to put an empty DWARFDataExtractor in here? Maybe this code 
should be:

```
const SectionList *section_list = module.GetSectionList();
  if (!section_list)
return nullptr;

  auto section_sp = section_list->FindSectionByType(section_type, true);
  if (!section_sp)
return nullptr;

  extractor.emplace();
  if (section_sp->GetSectionData(*extractor) == 0)
extractor = llvm::None;
  return extractor.getPointer();
```



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, eSectionTypeDWARFDebugAranges,

Why do we care about "getOrLoadArangesData()"? What not just "getArangesData()"?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:552
+  data.Clear();
+  m_obj_file->ReadSectionData(section_sp.get(), data);
 }

```
if (m_obj_file->ReadSectionData(section_sp.get(), data) == 0)
  data.Clear();
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner updated this revision to Diff 191510.
zturner marked an inline comment as done.
zturner added a comment.

Updated based on suggestions, including removal of the `m_dwarf_data` member 
variable which holds the contents of the `__DWARF` segment on MachO.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562

Files:
  lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -33,12 +33,6 @@
   if (section_list) {
 SectionSP section_sp(section_list->FindSectionByType(sect_type, true));
 if (section_sp) {
-  // See if we memory mapped the DWARF segment?
-  if (m_dwarf_data.GetByteSize()) {
-data.SetData(m_dwarf_data, section_sp->GetOffset(),
- section_sp->GetFileSize());
-return;
-  }
 
   if (m_obj_file->ReadSectionData(section_sp.get(), data) != 0)
 return;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -30,6 +30,7 @@
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
 
+#include "DWARFContext.h"
 #include "DWARFDataExtractor.h"
 #include "DWARFDefines.h"
 #include "DWARFIndex.h"
@@ -227,7 +228,6 @@
 
   virtual const lldb_private::DWARFDataExtractor &get_debug_abbrev_data();
   virtual const lldb_private::DWARFDataExtractor &get_debug_addr_data();
-  const lldb_private::DWARFDataExtractor &get_debug_aranges_data();
   const lldb_private::DWARFDataExtractor &get_debug_frame_data();
   virtual const lldb_private::DWARFDataExtractor &get_debug_info_data();
   const lldb_private::DWARFDataExtractor &get_debug_line_data();
@@ -458,11 +458,10 @@
   llvm::once_flag m_dwp_symfile_once_flag;
   std::unique_ptr m_dwp_symfile;
 
-  lldb_private::DWARFDataExtractor m_dwarf_data;
+  lldb_private::DWARFContext m_context;
 
   DWARFDataSegment m_data_debug_abbrev;
   DWARFDataSegment m_data_debug_addr;
-  DWARFDataSegment m_data_debug_aranges;
   DWARFDataSegment m_data_debug_frame;
   DWARFDataSegment m_data_debug_info;
   DWARFDataSegment m_data_debug_line;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -355,12 +355,13 @@
   << 32), // Used by SymbolFileDWARFDebugMap to
   // when this class parses .o files to
   // contain the .o file index/ID
-  m_debug_map_module_wp(), m_debug_map_symfile(NULL), m_data_debug_abbrev(),
-  m_data_debug_aranges(), m_data_debug_frame(), m_data_debug_info(),
-  m_data_debug_line(), m_data_debug_macro(), m_data_debug_loc(),
-  m_data_debug_ranges(), m_data_debug_rnglists(), m_data_debug_str(),
-  m_data_apple_names(), m_data_apple_types(), m_data_apple_namespaces(),
-  m_abbr(), m_info(), m_line(), m_fetched_external_modules(false),
+  m_debug_map_module_wp(), m_debug_map_symfile(NULL),
+  m_context(*objfile->GetModule()), m_data_debug_abbrev(),
+  m_data_debug_frame(), m_data_debug_info(), m_data_debug_line(),
+  m_data_debug_macro(), m_data_debug_loc(), m_data_debug_ranges(),
+  m_data_debug_rnglists(), m_data_debug_str(), m_data_apple_names(),
+  m_data_apple_types(), m_data_apple_namespaces(), m_abbr(), m_info(),
+  m_line(), m_fetched_external_modules(false),
   m_supports_DW_AT_APPLE_objc_complete_type(eLazyBoolCalculate), m_ranges(),
   m_unique_ast_type_map() {}
 
@@ -394,15 +395,6 @@
 
 void SymbolFileDWARF::InitializeObject() {
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO);
-  ModuleSP module_sp(m_obj_file->GetModule());
-  if (module_sp) {
-const SectionList *section_list = module_sp->GetSectionList();
-Section *section =
-section_list->FindSectionByName(GetDWARFMachOSegmentName()).get();
-
-if (section)
-  m_obj_file->ReadSectionData(section, m_dwarf_data);
-  }
 
   if (!GetGlobalPluginProperties()->IgnoreFileIndexes()) {
 DWARFDataExtractor apple_names, apple_namespaces

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:17
+static const DWARFDataExtractor *LoadOrGetSection(
+Module &module, const llvm::Optional 
&mapped_dwarf_data,
+SectionType section_type, llvm::Optional &extractor) {

zturner wrote:
> clayborg wrote:
> > is "mapped_dwarf_data" actually the entire file mmap'ed? If so rename this 
> > to "mapped_file_data"?
> It's the contents of the `__DWARF` segment on MachO, unless my reading of the 
> old code is wrong.
> 
> In the old code, `SymbolFileDWARF` would try to read this segment, and if it 
> existed set it to `SymbolFileDWARF::m_dwarf_data`.  Then, the 
> `ReadCachedSectionData` function would first check if this member had data in 
> it, and if so read the data from there instead of from the ObjectFile.
> 
> In this modified version of the patch, I call `SetDwarfData` with the 
> contents of this section, then pass it through to this function so that we 
> can perform the same logic.
remove "mapped_dwarf_data" arg



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:34-38
+  if (mapped_dwarf_data.hasValue()) {
+extractor->SetData(*mapped_dwarf_data, section_sp->GetOffset(),
+   section_sp->GetFileSize());
+return extractor.getPointer();
+  }

remove



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:46-49
+void DWARFContext::SetDwarfData(const DWARFDataExtractor &dwarf) {
+  m_dwarf_data = dwarf;
+}
+

zturner wrote:
> clayborg wrote:
> > Again, is this the entire file that is mapped?
> As mentioned elsewhere, this is the contents of `__DWARF`, which I didn't 
> understand the purpose of, so I was hesitant to make any drastic changes.  
> Perhaps you could clarify the significance of it now though and perhaps 
> suggest an alternative approach to what we're doing here.
remove



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:50
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, m_dwarf_data, 
eSectionTypeDWARFDebugAranges,

rename to "getArangesData()"? Would it be better to return an 
Optional?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:21
+  Module &m_module;
+  llvm::Optional m_dwarf_data;
+

clayborg wrote:
> What does m_dwarf_data contain? The entire file? Just the __DWARF segment for 
> mach-o?
Remove this now that we don't need it since Section::GetSectionData() will do 
the right thing already.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+

zturner wrote:
> labath wrote:
> > clayborg wrote:
> > > labath wrote:
> > > > Don't want to block progress here over this, but I just wanted to say 
> > > > that my impression was that this DwarfData business was a relict from 
> > > > the time when we re not mmapping the input object files. These days, we 
> > > > always mmap the input, and so I don't believe that should be necessary 
> > > > because ObjectFileMachO will automatically perform the DataBuffer 
> > > > slicing that you're doing here manually as a part of ReadSectionData.
> > > > 
> > > > Maybe one of the Apple folks could check whether this is really needed, 
> > > > because if it isn't, it will make your job easier here, as well as 
> > > > produce an API that is more similar to its llvm counterpart.
> > > We don't always mmap. We mmap if the file is local. We read the entire 
> > > file if the file is on a network drive. If we don't do this and we mmap a 
> > > NFS file, and that mount goes away, we crash the next time we access a 
> > > page that hasn't been paged in. So we don't know if stuff is actually 
> > > mmap'ed or not.
> > Yeah, I wasn't being fully correct here, but this distinction does not 
> > really matter here. Even if we don't mmap, we will end up with a full image 
> > of the file in memory, so anybody can slice any chunk of that file as he 
> > sees fit. Since ObjectFileMachO::ReadSectionData already implements this 
> > slicing, there's no advantage in slicing manually here.
> The parameter here represents the content of the `__DWARF` data segment on 
> MachO.  I don't really know what this is for or the implications of removing 
> it, so for now I left it identical to the original code with NFC, and planned 
> on addressing this in the future.
We can get rid of this after thinking about this.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:404-407
+if (section) {
   m_obj_file->ReadSectionData(section, m_dwarf_data);
+  m_dwarf_context.SetDwarfData(m_dwarf_data);
+}

Get rid of m_dwarf_data now that we don't need it? That will make subsequent 
patches easier.


===

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

We can get rid of the m_dwarf_data all together. As Pavel suggested, we used to 
not mmap the entire file, and now we do most of the time. The 
Section::GetSectionData() will ref count the mmap data in the data extractor 
and use the mmap already, so we should only support grabbing the contents via 
the Section::GetSectionData() and don't propagate this legacy code in the new 
DWARFContext.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner marked 5 inline comments as done.
zturner added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:17
+static const DWARFDataExtractor *LoadOrGetSection(
+Module &module, const llvm::Optional 
&mapped_dwarf_data,
+SectionType section_type, llvm::Optional &extractor) {

clayborg wrote:
> is "mapped_dwarf_data" actually the entire file mmap'ed? If so rename this to 
> "mapped_file_data"?
It's the contents of the `__DWARF` segment on MachO, unless my reading of the 
old code is wrong.

In the old code, `SymbolFileDWARF` would try to read this segment, and if it 
existed set it to `SymbolFileDWARF::m_dwarf_data`.  Then, the 
`ReadCachedSectionData` function would first check if this member had data in 
it, and if so read the data from there instead of from the ObjectFile.

In this modified version of the patch, I call `SetDwarfData` with the contents 
of this section, then pass it through to this function so that we can perform 
the same logic.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+  module.GetObjectFile()->ReadSectionData(section_sp.get(), *extractor);
+  return extractor.getPointer();

clayborg wrote:
> Use:
> 
> ```
>   lldb::offset_t Section::GetSectionData(DataExtractor &data);
> ```
> We might want to make the ObjectFile::ReadSectionData private so people don't 
> use it and add a doxygen comment to use the above API in Section?
Yes, I like that better.  Thanks for pointing it out.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:46-49
+void DWARFContext::SetDwarfData(const DWARFDataExtractor &dwarf) {
+  m_dwarf_data = dwarf;
+}
+

clayborg wrote:
> Again, is this the entire file that is mapped?
As mentioned elsewhere, this is the contents of `__DWARF`, which I didn't 
understand the purpose of, so I was hesitant to make any drastic changes.  
Perhaps you could clarify the significance of it now though and perhaps suggest 
an alternative approach to what we're doing here.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+

labath wrote:
> clayborg wrote:
> > labath wrote:
> > > Don't want to block progress here over this, but I just wanted to say 
> > > that my impression was that this DwarfData business was a relict from the 
> > > time when we re not mmapping the input object files. These days, we 
> > > always mmap the input, and so I don't believe that should be necessary 
> > > because ObjectFileMachO will automatically perform the DataBuffer slicing 
> > > that you're doing here manually as a part of ReadSectionData.
> > > 
> > > Maybe one of the Apple folks could check whether this is really needed, 
> > > because if it isn't, it will make your job easier here, as well as 
> > > produce an API that is more similar to its llvm counterpart.
> > We don't always mmap. We mmap if the file is local. We read the entire file 
> > if the file is on a network drive. If we don't do this and we mmap a NFS 
> > file, and that mount goes away, we crash the next time we access a page 
> > that hasn't been paged in. So we don't know if stuff is actually mmap'ed or 
> > not.
> Yeah, I wasn't being fully correct here, but this distinction does not really 
> matter here. Even if we don't mmap, we will end up with a full image of the 
> file in memory, so anybody can slice any chunk of that file as he sees fit. 
> Since ObjectFileMachO::ReadSectionData already implements this slicing, 
> there's no advantage in slicing manually here.
The parameter here represents the content of the `__DWARF` data segment on 
MachO.  I don't really know what this is for or the implications of removing 
it, so for now I left it identical to the original code with NFC, and planned 
on addressing this in the future.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:70
   SymbolFileDWARF *m_dwarf2Data;
+  lldb_private::DWARFContext *m_dwarf_context;
   CompileUnitColl m_compile_units;

clayborg wrote:
> Be nice if this can be a reference and be set in the ctor
Fair enough, I kept it as a pointer for parity with `SymbolFileDWARF* 
m_dwarf2Data`, but a reference is definitely better.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+

clayborg wrote:
> labath wrote:
> > Don't want to block progress here over this, but I just wanted to say that 
> > my impression was that this DwarfData business was a relict from the time 
> > when we re not mmapping the input object files. These days, we always mmap 
> > the input, and so I don't believe that should be necessary because 
> > ObjectFileMachO will automatically perform the DataBuffer slicing that 
> > you're doing here manually as a part of ReadSectionData.
> > 
> > Maybe one of the Apple folks could check whether this is really needed, 
> > because if it isn't, it will make your job easier here, as well as produce 
> > an API that is more similar to its llvm counterpart.
> We don't always mmap. We mmap if the file is local. We read the entire file 
> if the file is on a network drive. If we don't do this and we mmap a NFS 
> file, and that mount goes away, we crash the next time we access a page that 
> hasn't been paged in. So we don't know if stuff is actually mmap'ed or not.
Yeah, I wasn't being fully correct here, but this distinction does not really 
matter here. Even if we don't mmap, we will end up with a full image of the 
file in memory, so anybody can slice any chunk of that file as he sees fit. 
Since ObjectFileMachO::ReadSectionData already implements this slicing, there's 
no advantage in slicing manually here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:17
+static const DWARFDataExtractor *LoadOrGetSection(
+Module &module, const llvm::Optional 
&mapped_dwarf_data,
+SectionType section_type, llvm::Optional &extractor) {

is "mapped_dwarf_data" actually the entire file mmap'ed? If so rename this to 
"mapped_file_data"?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+  module.GetObjectFile()->ReadSectionData(section_sp.get(), *extractor);
+  return extractor.getPointer();

Use:

```
  lldb::offset_t Section::GetSectionData(DataExtractor &data);
```
We might want to make the ObjectFile::ReadSectionData private so people don't 
use it and add a doxygen comment to use the above API in Section?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:46-49
+void DWARFContext::SetDwarfData(const DWARFDataExtractor &dwarf) {
+  m_dwarf_data = dwarf;
+}
+

Again, is this the entire file that is mapped?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:21
+  Module &m_module;
+  llvm::Optional m_dwarf_data;
+

What does m_dwarf_data contain? The entire file? Just the __DWARF segment for 
mach-o?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+

labath wrote:
> Don't want to block progress here over this, but I just wanted to say that my 
> impression was that this DwarfData business was a relict from the time when 
> we re not mmapping the input object files. These days, we always mmap the 
> input, and so I don't believe that should be necessary because 
> ObjectFileMachO will automatically perform the DataBuffer slicing that you're 
> doing here manually as a part of ReadSectionData.
> 
> Maybe one of the Apple folks could check whether this is really needed, 
> because if it isn't, it will make your job easier here, as well as produce an 
> API that is more similar to its llvm counterpart.
We don't always mmap. We mmap if the file is local. We read the entire file if 
the file is on a network drive. If we don't do this and we mmap a NFS file, and 
that mount goes away, we crash the next time we access a page that hasn't been 
paged in. So we don't know if stuff is actually mmap'ed or not.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:41
+  void SetDwarfData(SymbolFileDWARF *dwarf2Data,
+lldb_private::DWARFContext *dwarf_context);
 

Store as a reference maybe? We must assume that the DWARFContext will be alive 
as long as this class is around. We would need to supply this in the 
constructor then. Is that possible?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:70
   SymbolFileDWARF *m_dwarf2Data;
+  lldb_private::DWARFContext *m_dwarf_context;
   CompileUnitColl m_compile_units;

Be nice if this can be a reference and be set in the ctor



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:690-691
static_cast(this));
 if (get_debug_info_data().GetByteSize() > 0) {
   m_info.reset(new DWARFDebugInfo());
   if (m_info) {

```
m_info.reset(new DWARFDebugInfo(m_dwarf_context));
```



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:693
   if (m_info) {
-m_info->SetDwarfData(this);
+m_info->SetDwarfData(this, &m_dwarf_context);
   }

revert


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+

Don't want to block progress here over this, but I just wanted to say that my 
impression was that this DwarfData business was a relict from the time when we 
re not mmapping the input object files. These days, we always mmap the input, 
and so I don't believe that should be necessary because ObjectFileMachO will 
automatically perform the DataBuffer slicing that you're doing here manually as 
a part of ReadSectionData.

Maybe one of the Apple folks could check whether this is really needed, because 
if it isn't, it will make your job easier here, as well as produce an API that 
is more similar to its llvm counterpart.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

I remember LLVM's DWARFContext being irritating to deal with when I was doing 
the DWARF 5 stuff.  The exact details have been swapped out of my memory, but I 
suspect it had something to do with needing to know whether I was in DWO mode 
or normal mode in order to snag the right section for some sort of 
cross-section lookup.  This is because DWARFContext is the dumping ground for 
all the sections, regardless of what kind of file we're looking at, and it's up 
to the user of its interfaces to know what mode it's in.
I will admit that having one dumping ground is convenient in some ways, 
especially when we were looking at object files that contained a mix of both 
normal and .dwo sections, but IIRC we don't emit mixed-mode object files 
anymore.

Not objecting to this patch, but it was the most convenient place to raise this 
particular grump, and something to think about as you progress.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner updated this revision to Diff 191421.
zturner added a comment.

I went ahead and just removed the const `get` version entirely, while changing 
the other function name to `getOrLoad` as you suggested.  I have another patch 
while I'll upload after this one that converts all of the rest of the functions 
(except for the virtual ones required for DWO support), and the const version 
of the function was literally never needed, except in one place that was itself 
dead code.  So, in the spirit of YAGNI, it's gone until it demonstrates a need.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562

Files:
  lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -30,6 +30,7 @@
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
 
+#include "DWARFContext.h"
 #include "DWARFDataExtractor.h"
 #include "DWARFDefines.h"
 #include "DWARFIndex.h"
@@ -227,7 +228,6 @@
 
   virtual const lldb_private::DWARFDataExtractor &get_debug_abbrev_data();
   virtual const lldb_private::DWARFDataExtractor &get_debug_addr_data();
-  const lldb_private::DWARFDataExtractor &get_debug_aranges_data();
   const lldb_private::DWARFDataExtractor &get_debug_frame_data();
   virtual const lldb_private::DWARFDataExtractor &get_debug_info_data();
   const lldb_private::DWARFDataExtractor &get_debug_line_data();
@@ -458,11 +458,11 @@
   llvm::once_flag m_dwp_symfile_once_flag;
   std::unique_ptr m_dwp_symfile;
 
+  lldb_private::DWARFContext m_dwarf_context;
   lldb_private::DWARFDataExtractor m_dwarf_data;
 
   DWARFDataSegment m_data_debug_abbrev;
   DWARFDataSegment m_data_debug_addr;
-  DWARFDataSegment m_data_debug_aranges;
   DWARFDataSegment m_data_debug_frame;
   DWARFDataSegment m_data_debug_info;
   DWARFDataSegment m_data_debug_line;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -355,12 +355,13 @@
   << 32), // Used by SymbolFileDWARFDebugMap to
   // when this class parses .o files to
   // contain the .o file index/ID
-  m_debug_map_module_wp(), m_debug_map_symfile(NULL), m_data_debug_abbrev(),
-  m_data_debug_aranges(), m_data_debug_frame(), m_data_debug_info(),
-  m_data_debug_line(), m_data_debug_macro(), m_data_debug_loc(),
-  m_data_debug_ranges(), m_data_debug_rnglists(), m_data_debug_str(),
-  m_data_apple_names(), m_data_apple_types(), m_data_apple_namespaces(),
-  m_abbr(), m_info(), m_line(), m_fetched_external_modules(false),
+  m_debug_map_module_wp(), m_debug_map_symfile(NULL),
+  m_dwarf_context(*objfile->GetModule()), m_data_debug_abbrev(),
+  m_data_debug_frame(), m_data_debug_info(), m_data_debug_line(),
+  m_data_debug_macro(), m_data_debug_loc(), m_data_debug_ranges(),
+  m_data_debug_rnglists(), m_data_debug_str(), m_data_apple_names(),
+  m_data_apple_types(), m_data_apple_namespaces(), m_abbr(), m_info(),
+  m_line(), m_fetched_external_modules(false),
   m_supports_DW_AT_APPLE_objc_complete_type(eLazyBoolCalculate), m_ranges(),
   m_unique_ast_type_map() {}
 
@@ -400,8 +401,10 @@
 Section *section =
 section_list->FindSectionByName(GetDWARFMachOSegmentName()).get();
 
-if (section)
+if (section) {
   m_obj_file->ReadSectionData(section, m_dwarf_data);
+  m_dwarf_context.SetDwarfData(m_dwarf_data);
+}
   }
 
   if (!GetGlobalPluginProperties()->IgnoreFileIndexes()) {
@@ -573,11 +576,6 @@
   return GetCachedSectionData(eSectionTypeDWARFDebugAddr, m_data_debug_addr);
 }
 
-const DWARFDataExtractor &SymbolFileDWARF::get_debug_aranges_data() {
-  return GetCachedSectionData(eSectionTypeDWARFDebugAranges,
-  m_data_debug_aranges);
-}
-
 const DWARFDataExtractor &SymbolFileDWARF::get_debug_frame_data() {
   return GetCachedSectionData(eSectionTypeDWARFDebugFrame, m_data_debug_frame);
 }
@@ -692,7 +690,7 @@
 if (get_debug_info_data().GetByteSize() > 0) {
   m_info.reset(new DWARFDebugInfo());
   if (m_info) {
-m_info->SetDwarfData(this);
+m_info->SetDwarfData(this, &m_dwarf_context);
   }
 }
   }
Index:

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:30
+
+  const DWARFDataExtractor *maybeLoadArangesData();
+

zturner wrote:
> JDevlieghere wrote:
> > Why do we need these two methods? I presume it's because one does work and 
> > the other doesn't? If I understand the code correctly, `LoadOrGetSection` 
> > will initialize the optional, so could you just check the optional to 
> > determine where we should try to do work or not?
> It's because one of them is `const` and a const version of the function 
> cannot do any work.  It's possible we won't ever need this, and we'll always 
> want a "get or create" function, but i can imagine scenarios where we have a 
> const reference or const pointer.
> 
> This is always a problem with these lazy type interfaces, because in that 
> case should you actually use `const_cast` or should you just fail and return 
> nullptr?  It's a tough call.  I chose to explicitly embed into the name 
> whether the function was doing work (e.g. `maybeLoad`) versus being purely a 
> getter that will never do work (e.g. `get`).
Aha, I missed that. nit about the name: how about `getArangesData` and 
`getOrLoadArangesData`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner marked an inline comment as done.
zturner added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:30
+
+  const DWARFDataExtractor *maybeLoadArangesData();
+

JDevlieghere wrote:
> Why do we need these two methods? I presume it's because one does work and 
> the other doesn't? If I understand the code correctly, `LoadOrGetSection` 
> will initialize the optional, so could you just check the optional to 
> determine where we should try to do work or not?
It's because one of them is `const` and a const version of the function cannot 
do any work.  It's possible we won't ever need this, and we'll always want a 
"get or create" function, but i can imagine scenarios where we have a const 
reference or const pointer.

This is always a problem with these lazy type interfaces, because in that case 
should you actually use `const_cast` or should you just fail and return 
nullptr?  It's a tough call.  I chose to explicitly embed into the name whether 
the function was doing work (e.g. `maybeLoad`) versus being purely a getter 
that will never do work (e.g. `get`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:30
+
+  const DWARFDataExtractor *maybeLoadArangesData();
+

Why do we need these two methods? I presume it's because one does work and the 
other doesn't? If I understand the code correctly, `LoadOrGetSection` will 
initialize the optional, so could you just check the optional to determine 
where we should try to do work or not?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59562/new/

https://reviews.llvm.org/D59562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner created this revision.
zturner added reviewers: JDevlieghere, aprantl, clayborg.
Herald added subscribers: jdoerfert, mgorny.

LLVM's DWARF parsing library has a class called `DWARFContext` which holds all 
of the various DWARF data sections and lots of other information.  LLDB's on 
the other hand stores all of this directly in `SymbolFileDWARF` / 
`SymbolFileDWARFDwo` and passes this interface around through the parsing 
library.  Obviously this is incompatible with a world where the low level 
interface does not depend on the high level interface, so we need to move 
towards a model similar to LLVM's - i.e. all of the context needed for low 
level parsing should be in a single class, and that class gets passed around.

This patch is a small incremental step towards achieving this.  The interface 
and internals deviate from LLVM's for technical reasons, but the high level 
idea is the same.  The goal is, eventually, to remove all occurrences of 
`SymbolFileDWARF` from the low level parsing code.

For now I've chosen a very simple section - the `.debug_aranges` section to 
move into `DWARFContext` while leaving everything else unchanged.  In the short 
term this is a bit confusing because now the information you need might come 
from either of 2 different locations.  But it's a huge refactor to do this all 
at once and runs a much higher risk of breaking things.  So I think it would be 
wise to do this in very small pieces.

TL;DR - No functional change


https://reviews.llvm.org/D59562

Files:
  lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -30,6 +30,7 @@
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
 
+#include "DWARFContext.h"
 #include "DWARFDataExtractor.h"
 #include "DWARFDefines.h"
 #include "DWARFIndex.h"
@@ -227,7 +228,6 @@
 
   virtual const lldb_private::DWARFDataExtractor &get_debug_abbrev_data();
   virtual const lldb_private::DWARFDataExtractor &get_debug_addr_data();
-  const lldb_private::DWARFDataExtractor &get_debug_aranges_data();
   const lldb_private::DWARFDataExtractor &get_debug_frame_data();
   virtual const lldb_private::DWARFDataExtractor &get_debug_info_data();
   const lldb_private::DWARFDataExtractor &get_debug_line_data();
@@ -458,11 +458,11 @@
   llvm::once_flag m_dwp_symfile_once_flag;
   std::unique_ptr m_dwp_symfile;
 
+  lldb_private::DWARFContext m_dwarf_context;
   lldb_private::DWARFDataExtractor m_dwarf_data;
 
   DWARFDataSegment m_data_debug_abbrev;
   DWARFDataSegment m_data_debug_addr;
-  DWARFDataSegment m_data_debug_aranges;
   DWARFDataSegment m_data_debug_frame;
   DWARFDataSegment m_data_debug_info;
   DWARFDataSegment m_data_debug_line;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -355,12 +355,13 @@
   << 32), // Used by SymbolFileDWARFDebugMap to
   // when this class parses .o files to
   // contain the .o file index/ID
-  m_debug_map_module_wp(), m_debug_map_symfile(NULL), m_data_debug_abbrev(),
-  m_data_debug_aranges(), m_data_debug_frame(), m_data_debug_info(),
-  m_data_debug_line(), m_data_debug_macro(), m_data_debug_loc(),
-  m_data_debug_ranges(), m_data_debug_rnglists(), m_data_debug_str(),
-  m_data_apple_names(), m_data_apple_types(), m_data_apple_namespaces(),
-  m_abbr(), m_info(), m_line(), m_fetched_external_modules(false),
+  m_debug_map_module_wp(), m_debug_map_symfile(NULL),
+  m_dwarf_context(*objfile->GetModule()), m_data_debug_abbrev(),
+  m_data_debug_frame(), m_data_debug_info(), m_data_debug_line(),
+  m_data_debug_macro(), m_data_debug_loc(), m_data_debug_ranges(),
+  m_data_debug_rnglists(), m_data_debug_str(), m_data_apple_names(),
+  m_data_apple_types(), m_data_apple_namespaces(), m_abbr(), m_info(),
+  m_line(), m_fetched_external_modules(false),
   m_supports_DW_AT_APPLE_objc_complete_type(eLazyBoolCalculate), m_ranges(),
   m_unique_ast_type_map() {}
 
@@ -400,8 +401,10 @@
 Section *section =
 section_list->FindSectionByName(GetDWARFMachOSegmentName()).get();
 
-if (section)
+if (section) {
   m_obj_file->Re