[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

DataExtractor is a copy of the one from LLDB from a while back and changes have been made to adapt it to llvm. DataExtractor was designed so that you can have one of them (like for .debug_info or any other DWARF section) and use this same extractor from multiple threads. This is why it is currently stateless.

One solution to allowing for correct error handling would be to replace the current "uint32_t *offset_ptr" arguments to DataExtractor decoding functions with a "DataCursor &Pos" where DataCursor is something like:

  class DataCursor {
    llvm::Expected<uint32_t> OffsetOrError;
  };

Then all of the state like the offset and any error state. Or it could be two members, an offset and an error.

The main issues is to not decrease parsing performance by introducing error checking on each byte. The current DataExtractor will return zeroes when things fail to extract, which is kind of tuned for DWARF since zeros are not valid DW_TAG, DW_AT,  DW_FORM and many other DWARF values. But it does allow for fast parsing. The idea was to quickly try and parse a bunch of data, and then make sure things are ok after doing some work (like parsing an entire DIE). So be careful with any changes to ensure DWARF parsing doesn't seriously regress.



================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:190
+    switch (E.Kind) {
+    case dwarf::DW_LLE_startx_length:
       // Pre-DWARF 5 has different interpretation of the length field. We have
----------------
We should switch the LEB functions in DataExtractor over to use the ones from:

```
#include <llvm/Support/LEB128.h
```

and use the:

```
inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
                              const uint8_t *end = nullptr,
                              const char **error = nullptr);

inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
                             const uint8_t *end = nullptr,
                             const char **error = nullptr);
```

functions... They have all the error checking and are quite efficient. since DataExtractor had been converted from LLDB over into LLVM, the person that moved DataExtractor into LLVM hadn't realized these functions (might have been me) were there when the move happened.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591