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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 8 inline comments as done.
labath added inline comments.


================
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),
----------------
clayborg wrote:
> 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.
I'd rather not do that. There aren't that many places that are constructing DIERefs -- I counted just four, and only one (the one in apple index) of them is passing llvm::None as the argument. I think it's better to have the person writing the code stop and think about what is the right value here, instead of just hoping that the default argument will be correct (which it won't be, in most cases).

Also having the arguments ordered this way makes it a nice logical progression. First you select the file, then the section in that file, and finally you give the offset in that section.


================
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,
----------------
clayborg wrote:
> can/should we still pass "const DIERef &ref" by value like we used to now that it is smaller?
Yeah, that's a good point.


================
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;
+
----------------
clayborg wrote:
> 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)};
> ```
> 
I don't think that's a good idea for two reasons:
- decoding is fairly simply in the non-debug-map case, but with a debug-map, the decoding is still relatively complicated, as you need to consult the debug map in order to find the right symbol file. If we don't move that part into the DIERef class, then we'll end up with a constructor that only works in some cases, which will be confusing. If we do move it, then we're letting information about SymbolFileDWARF and it's relationship to SymbolFileDWARFDebugMap leak into DIERef class.
- I wouldn't even say this is leaking information out of the DIERef class. SymbolFileDWARF is responsible for the conversion from a DIERef into a user_id_t (in SymbolFileDWARF::GetUID), and so it should also covert it back. For that to work, it obviously needs to be aware of the fields in the DIERef and their value ranges, but it is free to encode that information into a user_id_t any way it sees fit. (However, this reminds me that I should update GetUID to make use of the dwo_num abstraction. Right now, it works because the dwo_num is derived from the SymbolFile ID, but it would be cleaner to use dwo_num explicitly.)


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

https://reviews.llvm.org/D63428