[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDataExtractor.h:35-58
+  //------------------------------------------------------------------
+  /// Slide the data in the buffer so that access to the data will
+  /// start at offset \a offset.
+  ///
+  /// This is currently used to provide access to the .debug_types
+  /// section and pretend it starts at at offset of the size of the
+  /// .debug_info section. This allows a minimally invasive change to
----------------
This looks like a thing we could focus on next. Sliding the contents of the data buffer seems like a useful fiction, but the way it is implemented now, it relies on undefined behavior (out-of-range pointers, possibly even wrapping the address space) and it violates the invariants that we have some far been assuming about data extractors (namely, that for a DataExtractor, all offsets from 0 to GetByteSize()-1 are dereferencable). For example, for a "slid" data extractor:
- ValidOffset(0) will return true, even though that is clearly not the case
- Append(data2) will try to access data at *m_start and probably crash
- so will Checksum()

Since there seems to be a consensus (at least, nobody has proposed an alternative solution) that this is the way we should treat type units, I think it makes sense to spend some time to make sure we built it on solid foundations. To me, this sliding seems like a fundamental change in the nature of the data extractors, and so I think it should be done in the base DataExtractor class. So I'd propose to move this into a separate patch, where we can make sure that all other DataExtractor APIs do something sensible even if the extractor has been "slid".


https://reviews.llvm.org/D32167