[Lldb-commits] [PATCH] D63428: DWARF: Add "dwo_num" field to the DIERef class

[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: source/Plugins/SymbolFile/DWARF/DIERef.h:30
 
-  DIERef(Section s, llvm::Optional<dw_offset_t> u, dw_offset_t d)
-      : m_section(s), m_unit_offset(u.getValueOr(DW_INVALID_OFFSET)),
-        m_die_offset(d) {}
-
-  Section section() const { return static_cast<Section>(m_section); }
+  DIERef(llvm::Optional<uint32_t> num, Section s, dw_offset_t d)
+      : m_dwo_num(num.getValueOr(invalid_dwo_num)), m_section(s),
----------------
maybe make "llvm::Optional<uint32_t> num" the last arg and give it a llvm::None default argument value? Rename to dwo_num as well.


================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.h:49-50
+
+  unsigned m_dwo_num : 31;
   unsigned m_section : 1;
   dw_offset_t m_die_offset;
----------------
Can we use "uint32_t" here? Explicitly specify the size would be nice. 

Also we can avoid using invalid_dwo_num in this class if we do:

```
uint32_t m_dwo_num : 30;
uint32_t m_dwo_valid : 1;
uint32_t m_section : 1;
```


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp:19
 
-void DWARFIndex::ProcessFunctionDIE(llvm::StringRef name, DIERef ref,
-                                    DWARFDebugInfo &info,
+void DWARFIndex::ProcessFunctionDIE(llvm::StringRef name, const DIERef &ref,
+                                    SymbolFileDWARF &dwarf,
----------------
can/should we still pass "const DIERef &ref" by value like we used to now that it is smaller?


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFIndex.h:59
   /// vector.
-  void ProcessFunctionDIE(llvm::StringRef name, DIERef ref,
-                          DWARFDebugInfo &info,
+  void ProcessFunctionDIE(llvm::StringRef name, const DIERef &ref,
+                          SymbolFileDWARF &dwarf,
----------------
can/should we still pass "const DIERef &ref" by value like we used to now that it is smaller?


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1224-1230
+  DIERef::Section section =
+      uid >> 63 ? DIERef::Section::DebugTypes : DIERef::Section::DebugInfo;
+
+  llvm::Optional<uint32_t> dwo_num = uid >> 32 & 0x7fffffff;
+  if (*dwo_num == 0x7fffffff)
+    dwo_num = llvm::None;
+
----------------
Can we move the lldb::user_id_t code into DIERef now that it is really simple to decode? Too many details are leaking out of DIERef like its invalid dwo num enocding, where the section is in the user_id_t...

This code could be:
```
return DecodedUID{*this, DIERef(uid)};
```



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

https://reviews.llvm.org/D63428