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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

How invasive would it be to change this to not use an ostream but to instead use an in memory buffer? It would be useful to know where the use cases come from. Do we intend to use this inside of MC? Do we intend to output raw files? When outputting to MC is the space preallocated? Is there a reason that using an ostream would let us avoid a copy? If we can't avoid a copy or theres not an easy way to know the total size in advance then I'm pro using an std::vector<uint8_t> as the underlying storage mechanism instead of an ostream.



================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:29
+class FileWriter {
+  std::ostream &OS;
+  llvm::support::endianness ByteOrder;
----------------
Ideally we'd prefer to use a FileOutputBuffer or just a raw pointer I'd imagine. FileOutputBuffer isn't the right solution if you need support for writing into sections of other buffers. In that case you'd want something more raw. In llvm-objcopy we were forced to abstract between FileOutputBuffer and other output methods because they weren't compatible. FileOutputBuffer also requires you know the size in advance of writing which might be tricky when we have lots of LEB encoded values.


================
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);
----------------
clayborg wrote:
> 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?
Or ArrayRef<uint8_t> if this isn't a string. Generally I use "Data" to mean ArrayRef<uint8_t> and "String" to mean "StringRef" in these cases.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:22
+
+bool FileWriter::writeSLEB(int64_t S) {
+  uint8_t Bytes[32];
----------------
I think in general using bools to handle this sort of error doesn't make sense. You either want an error that gives more information or you just want to assert fail. For streams propogating the error is needed but for FileOutputBuffers you can perform all your checks in advance and then assert fail.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:24
+  uint8_t Bytes[32];
+  auto Length = encodeSLEB128(S, Bytes);
+  assert(Length < sizeof(Bytes));
----------------
Ah I didn't think about the use case here but returning an ArrayRef from encode might be better than just a length. Generally we try to avoid having lengths in a separate object from the data where possible.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:50-59
+bool FileWriter::fixup32(uint32_t U, off_t Offset) {
+  const off_t CurrOffset = tell();
+  if (CurrOffset == -1)
+    return false;
+  if (seek(Offset) != Offset)
+    return false;
+  if (!writeU32(U))
----------------
This function seems really off to me. When would any of these error conditions occur? Can't we assert fail on them rather than trying to handle the error?


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:61
+
+bool FileWriter::writeUnsigned(uint64_t U, size_t N) {
+  switch (N) {
----------------
Where is this needed? I'd prefer to make this a compile time error to be used incorrectly but if we really need variable sizes like this I want to understand the use case.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:76
+
+bool FileWriter::writeCStr(const char *CStr) {
+  assert(CStr != nullptr);
----------------
clayborg wrote:
> aprantl wrote:
> > This function looks like it's just asking for trouble :-)
> I will fix it by checking for nullptr.
It specifically adds the null terminator. I think `writeCStr` that takes a StringRef, writes the data, and then writes a null terminator afterwards makes sense for certian use cases.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:85
+  OS.seekp(Offset);
+  return OS.good() ? Offset : -1;
+}
----------------
An Expected should be used instead. Same comments about error handeling. Prefer ErrorOr, Expected, Error, and Optional where possible. Special values like -` should not be used.


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

https://reviews.llvm.org/D63828