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



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

Reply via email to