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

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 4 inline comments as done.
clayborg added a comment.

In D63828#1559914 <https://reviews.llvm.org/D63828#1559914>, @jakehehrlich wrote:

> How invasive would it be to change this to not use an ostream but to instead use an in memory buffer?


we use a std::stringstream when doing unit tests and this is a memory buffer. We will be mostly using std::ofstream objects for this when we save the file to disk. Not sure if that alleviates anything here. Changes here are very easy as we just need to write the FileWriter::WriteData(ArrayRef<uint8_t> Data) function and that is it. So very easy to change.

> It would be useful to know where the use cases come from. Do we intend to use this inside of MC?

Possibly if we write to ".gsym" sections later on?

> Do we intend to output raw files?

Yes, thus why I didn't use MC right now since I currently was only writing stand alone files. We also eventually want this to go in as a section in a file too.

> When outputting to MC is the space preallocated?

Right now the space doubles each time we go over if we use a std::ostringstrream, so it is efficient. If we are writing to file with std::ofstream, we don't have the issue.

> Is there a reason that using an ostream would let us avoid a copy?

Not that I am aware of. But easy to switch.

> 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.

Again, very easy to change. The biggest issue is we need something that can be used to write to a stand alone file _or_ to a section. So we can use the FileWriter class and conditionally write to a MC layer if we give it one, or abstract it by subclassing FileWriter and using the base class of FileWriter (where we would have a FileWriterMC or FileWriterBuffer).



================
Comment at: include/llvm/DebugInfo/GSYM/FileWriter.h:30
+class FileWriter {
+  std::ostream &OS;
+  llvm::support::endianness ByteOrder;
----------------
Yeah, we don't know if advance the size of everything. But happy to back this with whatever we want. We just need FileWriter::writeData(...) to do the work. Everything else uses that function.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:60
+    return false;
+  return seek(CurrOffset) == CurrOffset;
+}
----------------
Not sure what happens is the std::ostream isn't in a good state. Those could make it fail.


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:62
+}
+
+bool FileWriter::writeUnsigned(uint64_t U, size_t ByteSize) {
----------------
Remember the AddressOffsets table? It can be 8, 16, 32 or bit offsets when we write the address offsets that immediately follows the GSYM header. That is why it is in here. But I can remove this and add other write functions like:

```
bool FileWriter::write(ArrayRef<uint16_t> U16Array);
bool FileWriter::write(ArrayRef<uint32_t> U32Array);
bool FileWriter::write(ArrayRef<uint64_t> U64Array);
```

Those would byte swap all values in the array as they are written out if needed. Does that sound better?


================
Comment at: lib/DebugInfo/GSYM/FileWriter.cpp:77
+  OS.write((const char *)Data.data(), Data.size());
+  return OS.good();
+}
----------------
See my previous comments on why using StringRef was not a great idea.


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

https://reviews.llvm.org/D63828