[PATCH] D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 2 inline comments as done.
clayborg added inline comments.


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:25
+
+/// A simplified binary data writer class that doesn't require targets, target
+/// definitions, architectures, or require any other optional compile time
----------------
JDevlieghere wrote:
> It seems like now the `FileWriter` is mostly concerned with the byte order. I'd update this comment saying that it's a wrapper around `raw_pwrite_stream` with convenience methods to deal with endianness.  
Eventually this class will probably encapsulate a ASMPrinter when we need to write the file to a section. So I am not sure how much was want to change this description as it still fits. raw_pwrite_stream is what we are using right now, but it might not be in the future. So I believe this comment still is true. What do you think?


================
Comment at: lib/DebugInfo/GSYM/Range.cpp:71
+  Start = StartAddr;
+  End = StartAddr + Size;
+}
----------------
JDevlieghere wrote:
> Should we check that the new offsets are valid? 
There in an assertion in DataExtractor::getULEB128() that will validate the offset is valid when assertions are enable, though I am not a fan of that since it can crash the program if you have a truncated GSYM file. 

It is hard to pre-validate if there is enough data for a ULEB128 number in the stream at offset "Offset" since it can be one byte, or many bytes, and without parsing the data first, we can't know the length it will need and it seems like DataExtractor::getULEB128 should handle this gracefully instead of crashing with an assert, or worse yet, just checking that the initial offset is valid, an happily walk off the end of the array like it does.

The DataExtractor::getULEB128 looks like:

```
uint64_t DataExtractor::getULEB128(uint32_t *offset_ptr) const {
  assert(*offset_ptr <= Data.size());

  const char *error;
  unsigned bytes_read;
  uint64_t result = decodeULEB128(
      reinterpret_cast<const uint8_t *>(Data.data() + *offset_ptr), &bytes_read,
      reinterpret_cast<const uint8_t *>(Data.data() + Data.size()), &error);
  if (error)
    return 0;
  *offset_ptr += bytes_read;
  return result;
}
```

I would rather see no assert:

```
uint64_t DataExtractor::getULEB128(uint32_t *offset_ptr) const {
  if (*offset_ptr >= Data.size());
    return 0;
```

We could check if "Offset" is a valid offset before each call to Data.getULEB128(). Thoughts?


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

https://reviews.llvm.org/D63828