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

[Greg Clayton via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:41
+  bool writeUnsigned(uint64_t Value, size_t N);
+  bool writeData(const void *Src, size_t SrcLen);
+  bool writeCStr(const char *CStr);
----------------
aprantl wrote:
> This should take a StringRef
So any data you want to write, like a uint8_t or a uint16_t, you need to put it into a StringRef just to write it out instead of just taking the address of a local variable and passing sizeof()? Not sure that is a great interface? All integer and data writes go through this function. StringRef is designed to hold C strings, not just raw data. llvm::ArrayRef<uint8_t> maybe?


================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:42
+  bool writeData(const void *Src, size_t SrcLen);
+  bool writeCStr(const char *CStr);
+  bool fixup32(uint32_t Value, off_t Offset);
----------------
aprantl wrote:
> This probably shouldn't exist :-)
I believe it should. It clearly states we are writing a NULL terminated C string. 

If we pass a StringRef the simple cases are clear:
- StringRef("hello"): write out "hello\0"
- StringRef("")? Write out "\0"

But there are many things that are not as clear if we pass in:
- StringRef()? Write out nothing since it has no data? Or write out "\0"?
- StringRef("hello\0world\0")? Just write out "hello\0" since that is the first C string?
- StringRef("hello\0") (where size includes the NULL termination byte) write out "hello\0". We will need to check if the string ends with '\0', or contains any '\0' for so we avoid emitting an extra NULL character





================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:14
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
----------------
aprantl wrote:
> Why do you need this? Is there no abstraction in support for what you need?
I'll remove it.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:76
+
+bool FileWriter::writeCStr(const char *CStr) {
+  assert(CStr != nullptr);
----------------
aprantl wrote:
> This function looks like it's just asking for trouble :-)
I will fix it by checking for nullptr.


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

https://reviews.llvm.org/D63828