Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges

[clayborg]

Summary

The full GSYM patch started with: https://reviews.llvm.org/D53379

This patch add the ability to encode data using the new llvm::gsym::FileWriter class.

FileWriter is a simplified binary data writer class that doesn't require targets, target definitions, architectures, or require any other optional compile time libraries to be enabled via the build process. This class needs the ability to seek to different spots in the binary data that it produces to fix up offsets and sizes in GSYM data. It currently uses std::ostream over llvm::raw_ostream because llvm::raw_ostream doesn't support seeking which is required when encoding and decoding GSYM data.

AddressRange objects are encoded and decoded to be relative to a base address. This will be the FunctionInfo's start address if the AddressRange is directly contained in a FunctionInfo, or a base address of the containing parent AddressRange or AddressRanges. This allows address ranges to be efficiently encoded using ULEB128 encodings as we encode the offset and size of each range instead of full addresses. This also makes encoded addresses easy to relocate as we just need to relocate one base address.

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

42

This should take a StringRef

43

This probably shouldn't exist :-)

lib/DebugInfo/GSYM/FileWriter.cpp

15

Why do you need this? Is there no abstraction in support for what you need?

77

This function looks like it's just asking for trouble :-)

lib/DebugInfo/GSYM/Range.cpp

86

range-based for?

unittests/DebugInfo/GSYM/GSYMTest.cpp

410

.

[clayborg]

include/llvm/DebugInfo/GSYM/FileWriter.h

42

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?

43

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

lib/DebugInfo/GSYM/FileWriter.cpp

15

I'll remove it.

77

I will fix it by checking for nullptr.

[clayborg]

Switched FileWriter::writeData() to use a llvm::ArrayRef<uint8_t> instead of pointer and size.
Address other review comments.

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

43

Let's ask the underlying question: Which of the above use-cases does a GSYM writer need, and why isn't the StringRef case sufficient?

[clayborg]

include/llvm/DebugInfo/GSYM/FileWriter.h

44

It needs both the ability to write raw data (for raw data after being swapped and for internal implementation) and NULL terminated C strings (for string table).

[jakehehrlich]

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.

include/llvm/DebugInfo/GSYM/FileWriter.h

30

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.

42

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.

lib/DebugInfo/GSYM/FileWriter.cpp

23

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.

25

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.

51–60

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?

62

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.

77

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.

86

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.

[clayborg]

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

include/llvm/DebugInfo/GSYM/FileWriter.h

31

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.

lib/DebugInfo/GSYM/FileWriter.cpp

61

Not sure what happens is the std::ostream isn't in a good state. Those could make it fail.

63

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?

78

See my previous comments on why using StringRef was not a great idea.

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

44

Now that you changed the other API to take an ArrayRef, there should be no problem changing this method to:

bool writeNullTerminated(StringRef s);

?

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

35

This whole interface needs Doxygen comments.

36

What are the bool return values indicating? If it's error handling — is there a better way of doing this?

lib/DebugInfo/GSYM/FileWriter.cpp

13

#include <cassert>

[clayborg]

include/llvm/DebugInfo/GSYM/FileWriter.h

35

Agreed, but I am not going to do it before we agree on the API.

I just didn't know if this class was going to be thrown out in favor of the MC layer. I couldn't figure out how to use it for raw files so I made this class, but I figured someone with more llvm expertise might let me know how to do it or how to do it another way.

I figured FileWriter would need changes so I just want to iterate on this and get to the "lgtm once we have doxygen comments", then I will add full docs. Is that ok?

36

bool for success. We can remove this and make it void, or match what the MC layer does. Do we want to just switch all return values to "void" an dhave the user periodically check in this the FileWriter class and ask if it is good? Error checking on each write seems like overkill, so I tried to keep it simple with a bool for now. Let me know what you think.

44

I can just switch this to StringRef and have the user be on the hook for not passing in a StringRef with embedded NULL characters. We can always write the entire string and always add a NULL. Is that ok?

[clayborg]

Switch FileWriter::writeCStr to use llvm::StringRef. Addressed other review comments.

[aprantl]

JF is currently working on making BitStream error-safe over in https://reviews.llvm.org/D63518. Perhaps there are some patterns that are worth copying.

[JDevlieghere]

Most of my concerns have already been noted by other reviewers, in particular with regards to error handling. Just a small nit inline.

lib/DebugInfo/GSYM/FileWriter.cpp

40

You're already using static_cast etc, so let's use reinterpret_cast here? Applies to the rest of the review as well.

[clayborg]

In D63828#1561085, @aprantl wrote:

JF is currently working on making BitStream error-safe over in https://reviews.llvm.org/D63518. Perhaps there are some patterns that are worth copying.

Any pattern in particular? I would rather not have to try and take an error after every write. The ASMPrinter returns void. Do we even need a return value? We could store any errors internally in a contained lldb::Error that must be checked before the object is destructed?

[clayborg]

Use reinterpret_cast instead of C casts.

[aprantl]

The important part is that the error must be checked (which isn't enforceable through bool returns). Whether it's after each function call or later, you probably have a better idea about.

[clayborg]

Added llvm::Error as a member variable of FileWriter. Each method that does anything with the stream will check if there is an error and return if the FileWriter has encountered an error. Added:

llvm::Error FileWriter::takeError();

To allow clients to check errors after a series of method calls on FileWriter objects. The error must be checked or taken before the FileWriter object destructs.

[clayborg]

Update this patch to compile after r364634.

[jakehehrlich]

This all LGTM now. Please wait long enough for others to get their say but overall I'm happy now.

include/llvm/DebugInfo/GSYM/FileWriter.h

68

Personal preference, can be this be a conversion to bool instead?

[aprantl]

We need to come up with a better error handling strategy before this can be landed.

lib/DebugInfo/GSYM/FileWriter.cpp

69

I'm sure this can be improved. Currently this function checks for errors, but then silently discards them and returns void.

[MaskRay]

It currently uses std::ostream over llvm::raw_ostream because llvm::raw_ostream doesn't support seeking which is required when encoding and decoding GSYM data.

There is raw_fd_ostream::seek. You just need to make sure raw_fd_ostream::supportsSeeking() returns true when you open it.

#include <iostream> is Forbidden.

lib/DebugInfo/GSYM/FileWriter.cpp

96

I think calling hasError() in every write function is less ideal. Errors can be checked after a batch write in the call sites.

98

Write on empty data just works. No need to check emptiness.

132

If Align is a power of 2, you can use (Offset+Align-1) & -Align.

137

If you use raw_fd_ostream, there is raw_ostream::write_zeros.

[clayborg]

lib/DebugInfo/GSYM/FileWriter.cpp

96

So the question really becomes do we want the first error to stick, or just let the stream get into a bad state and then report some random error later. Error checking is quite cheap (a pointer check) so I don't think the overhead is too much. I would much rather know an accurate first error over just getting some "failed to write" error later when something else caused the error in the first place. Thoughts?

98

The main worry was passing nullptr for the data.

132

It isn't required to be a power of 2 currently.

[clayborg]

In D63828#1563337, @MaskRay wrote:

It currently uses std::ostream over llvm::raw_ostream because llvm::raw_ostream doesn't support seeking which is required when encoding and decoding GSYM data.

There is raw_fd_ostream::seek. You just need to make sure raw_fd_ostream::supportsSeeking() returns true when you open it.

The main issue is being able to write to a memory buffer so we don't need to create a temp file on disk when creating sections or doing unit tests. The only raw_ostream that supports seeking is the fd class, none of the other do and the docs for raw_ostream state that seek isn't supported. What we would need is a few virtual methods on raw_ostream so that any raw_ostream can support seeking, but I am not sure people would be thrilled by this. For unit tests I was wanting to use raw_string_ostream, but again, no seek is available on the raw_ostream base class.

#include <iostream> is Forbidden.

It is forbidden in header files only.

[clayborg]

Don't #include <iostream>

[clayborg]

So what is the verdict on FileWriter? Do we want real error handling like is currently coded, or just be able to check sometime later and find out that something went wrong? If we just want to check that something is wrong, we don't need to maintain a llvm::Error, we can just check the stream at any point and return a generic error that means nothing. If this is what we do, I am not sure I see the point of adding the error handling. The current solution calls hasError() which checks if the llvm::Error contains a ErrorInfoBase pointer so the call is pretty efficient. And the first thing to cause the error will stay in the error object.

One vote against going with raw_ostream based streams is there is no error handling in the base class. Also no seek support in the base class. raw_fd_stream has std::error_code type handling, and then requires us to use only file descriptor based streams (no memory based streams for unit tests or quickly making GSYMs in memory).

Comments?

[MaskRay]

In D63828#1567323, @clayborg wrote:

So what is the verdict on FileWriter? Do we want real error handling like is currently coded, or just be able to check sometime later and find out that something went wrong? If we just want to check that something is wrong, we don't need to maintain a llvm::Error, we can just check the stream at any point and return a generic error that means nothing. If this is what we do, I am not sure I see the point of adding the error handling. The current solution calls hasError() which checks if the llvm::Error contains a ErrorInfoBase pointer so the call is pretty efficient. And the first thing to cause the error will stay in the error object. Without the error checking, FileWriter seems to just forward IO operations to the underlying std::ostream. I wonder if it would be easier to review the other components of GSYM by inlining those FileWriter use sites.

Others may have different opinions but my feeling is that FileWriter does lots of duplicate work that has been done in raw_ostream. It tries to catch errors but I don't see how errors are handled. It may become clearer when other components of GSYM are checked in. Before that, without concrete use cases, it may be difficult to suggest what FileWriter should do.

One vote against going with raw_ostream based streams is there is no error handling in the base class. Also no seek support in the base class. raw_fd_stream has std::error_code type handling, and then requires us to use only file descriptor based streams (no memory based streams for unit tests or quickly making GSYMs in memory).

Comments?

So the problem is that raw_ostream doesn't support seek. raw_fd_ostream supports seek but a fd-based stream is not desired for GSYM. I think we can ask people who have worked on raw_ostream what is the best way going forward.

If it cannot be decidedly shortly, I wonder if it will be easier to move forward with GSYM by inlining the FileWriter calls into other components. You can add the abstraction layer back later when the exceptations of the error checking become clearer after more components are checked in.

[clayborg]

In D63828#1567764, @MaskRay wrote:

In D63828#1567323, @clayborg wrote:

So what is the verdict on FileWriter? Do we want real error handling like is currently coded, or just be able to check sometime later and find out that something went wrong? If we just want to check that something is wrong, we don't need to maintain a llvm::Error, we can just check the stream at any point and return a generic error that means nothing. If this is what we do, I am not sure I see the point of adding the error handling. The current solution calls hasError() which checks if the llvm::Error contains a ErrorInfoBase pointer so the call is pretty efficient. And the first thing to cause the error will stay in the error object. Without the error checking, FileWriter seems to just forward IO operations to the underlying std::ostream. I wonder if it would be easier to review the other components of GSYM by inlining those FileWriter use sites.

Others may have different opinions but my feeling is that FileWriter does lots of duplicate work that has been done in raw_ostream. It tries to catch errors but I don't see how errors are handled. It may become clearer when other components of GSYM are checked in. Before that, without concrete use cases, it may be difficult to suggest what FileWriter should do.

We have existing cases right now. We are writing binary data in either byte order to a stream. Not sure what more we need here? If we end up writing to a an ASMPrinter later, FileWriter will be the abstraction layer so that GSYM code doesn't need to know the details of wether we are writing to a section in a file, or just a blob of data for a stand alone file.

One vote against going with raw_ostream based streams is there is no error handling in the base class. Also no seek support in the base class. raw_fd_stream has std::error_code type handling, and then requires us to use only file descriptor based streams (no memory based streams for unit tests or quickly making GSYMs in memory).

Comments?

So the problem is that raw_ostream doesn't support seek. raw_fd_ostream supports seek but a fd-based stream is not desired for GSYM. I think we can ask people who have worked on raw_ostream what is the best way going forward.

If it cannot be decidedly shortly, I wonder if it will be easier to move forward with GSYM by inlining the FileWriter calls into other components. You can add the abstraction layer back later when the exceptations of the error checking become clearer after more components are checked in.

Not sure what would get inlined. We need a way to write integers of varying sizes and byte orders into some data. raw_ostream doesn't support this at all right? What would get inlined? We need a way to encode and decode the objects into a stream and we need to be able to do this to a raw buffer and later to a ASMPrinter. I would rather keep FileWriter here as that layer. std::ostream is working fine for now, and raw_ostream seems to have been made really simple on purpose and doesn't seem to be the right layer for this as raw_ostream is made for logging text.

[clayborg]

I am happy to do what is needed. Anyone have any comments on my last comments? We just need a solid direction that everyone thinks we should follow.

[clayborg]

Ping for comments

[clayborg]

I need some feedback here. Is the general consensus that we need to modify raw_ostream to support seeking or continue with the current FileWriter?

[jakehehrlich]

I think teaching more raw_ostreams about seeking is probably right. I started to propose a solution but it boiled down to make our own abstractions over existing ostreams and what amounts to raw_string_ostream. I think raw_string_ostream and raw_os_ostream should both support seeking

[clayborg]

Made a patch to support seeking in llvm::raw_ostream: https://reviews.llvm.org/D65096

[clayborg]

Changes:

• switch FileWriter over to use llvm::raw_pwrite_stream to avoid needing to use std::ostream
• Remove error handlings from FileWrite since neither llvm::raw_pwrite_stream nor llvm::raw_ostream has any
• Remove the write unsigned integer function that took a byte size to simplify logic
• Use SmallString +raw_svector_ostream in places that used to use std::ostringstream

[clayborg]

• Remove comments that mention errors in GSYMTest.cpp
• Remove header includes that were no longer needed

[clayborg]

Friendly ping. Would love to get GSYM moving if possible.

[JDevlieghere]

Hi Greg, sorry about the delay. I have a few small nits about documentation and one question about error handling in the decode method, but otherwise this looks good to me.

include/llvm/DebugInfo/GSYM/FileWriter.h

26

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.

include/llvm/DebugInfo/GSYM/Range.h

60

Can you add an @{ and @} around this to group the comment? (http://www.doxygen.nl/manual/grouping.html)

97

Can you add an @{ and @} around this to group the comment?

lib/DebugInfo/GSYM/Range.cpp

71

Should we check that the new offsets are valid?

[clayborg]

include/llvm/DebugInfo/GSYM/FileWriter.h

26

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?

lib/DebugInfo/GSYM/Range.cpp

71

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?

[clayborg]

Added and around grouped comments as suggested. Waiting for responses on previous comments about changing FileWriter description and calling DataExtractor::getULEB128().

[clayborg]

ping

[clayborg]

ping

[clayborg]

anyone have any time to look at this again? Would love to get things moving.

[clayborg]

ping

[clayborg]

friendly ping

[clayborg]

Can anyone take a look please? Another week, no comments.

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

35

Looks like the comments are still missing.

44

Ideally, then the method should be called writeNullTerminated or something to that end.

[clayborg]

• added full FileWriter doxygen comments
• renamed writeCStr to writeNullTerminated
• update all DataExtractor offsets to be uint64_t to match recent DataExtractor changes

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

38

/// Write a ...

41

/// \param Value

43

ditto...

89

additional?

[clayborg]

• remove function name from doxygen comments
• change @param to \param
• fix typos

[clayborg]

include/llvm/DebugInfo/GSYM/FileWriter.h

41

Yikes, there is a large mix of both @param and \param in the code...

[clayborg]

Let's ask the underlying question: Which of the above use-cases does a GSYM writer need, and why isn't the StringRef case sufficient?

Couldn't tell if I needed to still comment on this, but here goes. The function:

void FileWriter::writeData(llvm::ArrayRef<uint8_t> Data)

will write exactly the bytes that are in Data. The function:

void FileWriter::writeNullTerminated(llvm::StringRef Str);

Will write out a the contents of the Str as a NULL terminated C string where the NULL termination doesn't need to be part of the string. Using StringRef for data means we usually have to use static cast:

writer.writeNullTerminated(StringRef(static_cast<const char *>(...), ...);

So GSYM uses the writeNullTerminated for standard C strings in the string table which include the NULL termination, and uses writeData for raw data when writing out the swapped values.

[aprantl]

All of my comments have been addressed.

[aprantl]

include/llvm/DebugInfo/GSYM/FileWriter.h

41

\param is the preferred spelling in LLVM code (I thought I had replaced all remaining @'s with backslashes a while ago).

[MaskRay]

include/llvm/DebugInfo/GSYM/FileWriter.h

6

The license notice is wrong.

lib/DebugInfo/GSYM/FileWriter.cpp

7

The licence notice is wrong.

[MaskRay]

include/llvm/DebugInfo/GSYM/FileWriter.h

19

sys/types.h ?

include/llvm/DebugInfo/GSYM/FileWriter.h

+//===- FileWriter.h ---------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_DEBUGINFO_GSYM_FILEWRITER_H
+#define LLVM_DEBUGINFO_GSYM_FILEWRITER_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/Error.h"
+
+#include <iosfwd>
+#include <stddef.h>
+#include <stdint.h>
+#include <sys/types.h>
+
+namespace llvm {
+namespace gsym {
+
+/// A simplified binary data writer class that doesn't require targets, target
+/// definitions, architectures, or require any other optional compile time
+/// libraries to be enabled via the build process. This class needs the ability
+/// to seek to different spots in the binary stream that is produces to fixup
+/// offsets and sizes. 
+///
+/// The class has a member variable named "Error" which will bet set to the first
+/// error when any methods are called. This error must be checked after using this
+/// class before the object is destroyed. This allows this object to be used in
+/// high performance loops when writing data to the stream, and then checked at a 
+/// later time before the object is destroyed.
+class FileWriter {
+  std::ostream &OS;
+  llvm::support::endianness ByteOrder;
+  llvm::Error Error = llvm::Error::success(); 
+public:
+  FileWriter(std::ostream &S, llvm::support::endianness B)
+      : OS(S), ByteOrder(B) {}
+  ~FileWriter();
+  void writeU8(uint8_t Value);
+  void writeU16(uint16_t Value);
+  void writeU32(uint32_t Value);
+  void writeU64(uint64_t Value);
+  void writeSLEB(int64_t Value);
+  void writeULEB(uint64_t Value);
+  void writeUnsigned(uint64_t Value, size_t ByteSize);
+  void writeData(llvm::ArrayRef<uint8_t> Data);
+  void writeCStr(llvm::StringRef Str);
+  void fixup32(uint32_t Value, off_t Offset);
+  void alignTo(size_t Align);
+  void seek(off_t Offset);
+  off_t tell();
+
+  llvm::Error takeError() {
+    if (hasError())
+      return std::move(Error);
+    return Error::success();
+  }
+
+private:
+  FileWriter(const FileWriter &rhs) = delete;
+  void operator=(const FileWriter &rhs) = delete;
+  bool hasError();
+};
+
+} // namespace gsym
+} // namespace llvm
+
+#endif // #ifndef LLVM_DEBUGINFO_GSYM_FILEWRITER_H

include/llvm/DebugInfo/GSYM/Range.h

 #include <vector>
 
 #define HEX8(v) llvm::format_hex(v, 4)
 #define HEX16(v) llvm::format_hex(v, 6)
 #define HEX32(v) llvm::format_hex(v, 10)
 #define HEX64(v) llvm::format_hex(v, 18)
 
 namespace llvm {
+class DataExtractor;
 class raw_ostream;
 
 namespace gsym {
 
+class FileWriter;
+
 /// A class that represents an address range. The range is specified using
 /// a start and an end address.
 struct AddressRange {
   uint64_t Start;
   uint64_t End;
   AddressRange() : Start(0), End(0) {}
   AddressRange(uint64_t S, uint64_t E) : Start(S), End(E) {}
   uint64_t size() const { return End - Start; }
   bool contains(uint64_t Addr) const { return Start <= Addr && Addr < End; }
   bool intersects(const AddressRange &R) const {
     return Start < R.End && R.Start < End;
   }
 
   bool operator==(const AddressRange &R) const {
     return Start == R.Start && End == R.End;
   }
   bool operator!=(const AddressRange &R) const {
     return !(*this == R);
   }
   bool operator<(const AddressRange &R) const {
     return std::make_pair(Start, End) < std::make_pair(R.Start, R.End);
   }
+  /// AddressRange objects are encoded and decoded to be relative to a base
+  /// address. This will be the FunctionInfo's start address if the AddressRange
+  /// is directly contained in a FunctionInfo, or a base address of the
+  /// containing parent AddressRange or AddressRanges. This allows address
+  /// ranges to be efficiently encoded using ULEB128 encodings as we encode the
+  /// offset and size of each range instead of full addresses. This also makes
+  /// encoded addresses easy to relocate as we just need to relocate one base
+  /// address.
+  void decode(DataExtractor &Data, uint64_t BaseAddr, uint32_t &Offset);
+  void encode(FileWriter &O, uint64_t BaseAddr) const;
 };
 
 raw_ostream &operator<<(raw_ostream &OS, const AddressRange &R);
 
 /// The AddressRanges class helps normalize address range collections.
 /// This class keeps a sorted vector of AddressRange objects and can perform
 /// insertions and searches efficiently. The address ranges are always sorted
 /// and never contain any invalid or empty address ranges. This allows us to
bool operator==(const AddressRanges &RHS) const {
     return Ranges == RHS.Ranges;
   }
   const AddressRange &operator[](size_t i) const {
     assert(i < Ranges.size());
     return Ranges[i];
   }
   Collection::const_iterator begin() const { return Ranges.begin(); }
   Collection::const_iterator end() const { return Ranges.end(); }
+
+  /// Address ranges are decoded and encoded to be relative to a base address.
+  /// See the AddressRange comment for the encode and decode methods for full
+  /// details.
+  void decode(DataExtractor &Data, uint64_t BaseAddr, uint32_t &Offset);
+  void encode(FileWriter &O, uint64_t BaseAddr) const;
 };
 
 raw_ostream &operator<<(raw_ostream &OS, const AddressRanges &AR);
 
 } // namespace gsym
 } // namespace llvm
 
 #endif // #ifndef LLVM_DEBUGINFO_GSYM_RANGE_H

lib/DebugInfo/GSYM/CMakeLists.txt

 add_llvm_library(LLVMDebugInfoGSYM
+  FileWriter.cpp
   FunctionInfo.cpp
   InlineInfo.cpp
   Range.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/GSYM
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo
   )

lib/DebugInfo/GSYM/FileWriter.cpp

+//===- FileWriter.cpp -------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/DebugInfo/GSYM/FileWriter.h"
+#include "llvm/Support/LEB128.h"
+#include <cassert>
+#include <ostream>
+
+using namespace llvm;
+using namespace gsym;
+
+FileWriter::~FileWriter() { OS.flush(); }
+
+bool FileWriter::hasError() {
+  return (bool)Error;
+}
+
+void FileWriter::writeSLEB(int64_t S) {
+  uint8_t Bytes[32];
+  auto Length = encodeSLEB128(S, Bytes);
+  assert(Length < sizeof(Bytes));
+  return writeData(ArrayRef<uint8_t>(Bytes, Length));
+}
+
+void FileWriter::writeULEB(uint64_t U) {
+  uint8_t Bytes[32];
+  auto Length = encodeULEB128(U, Bytes);
+  assert(Length < sizeof(Bytes));
+  return writeData(ArrayRef<uint8_t>(Bytes, Length));
+}
+
+void FileWriter::writeU8(uint8_t U) {
+  return writeData(ArrayRef<uint8_t>(&U, sizeof(U)));
+}
+
+void FileWriter::writeU16(uint16_t U) {
+  const uint16_t Swapped = support::endian::byte_swap(U, ByteOrder);
+  return writeData(ArrayRef<uint8_t>(
+      reinterpret_cast<const uint8_t *>(&Swapped), sizeof(Swapped)));
+}
+
+void FileWriter::writeU32(uint32_t U) {
+  const uint32_t Swapped = support::endian::byte_swap(U, ByteOrder);
+  return writeData(ArrayRef<uint8_t>(
+      reinterpret_cast<const uint8_t *>(&Swapped), sizeof(Swapped)));
+}
+
+void FileWriter::writeU64(uint64_t U) {
+  const uint64_t Swapped = support::endian::byte_swap(U, ByteOrder);
+  return writeData(ArrayRef<uint8_t>(
+      reinterpret_cast<const uint8_t *>(&Swapped), sizeof(Swapped)));
+}
+
+void FileWriter::fixup32(uint32_t U, off_t Offset) {
+  const off_t SavedOffset = tell();
+  if (SavedOffset == -1)
+    return;
+  seek(Offset);
+  if (hasError())
+    return;
+  writeU32(U);
+  if (hasError())
+    return;
+  seek(SavedOffset);
+}
+
+void FileWriter::writeUnsigned(uint64_t U, size_t ByteSize) {
+  switch (ByteSize) {
+  case 1:
+    writeU8(static_cast<uint8_t>(U));
+    break;
+  case 2:
+    writeU16(static_cast<uint16_t>(U));
+    break;
+  case 4:
+    writeU32(static_cast<uint32_t>(U));
+    break;
+  case 8:
+    writeU64(U);
+    break;
+  default:
+    Error = llvm::make_error<llvm::StringError>(
+        "unsupported byte size in FileWriter::writeUnsigned",
+        llvm::inconvertibleErrorCode());
+    break;
+  }
+}
+
+void FileWriter::writeData(llvm::ArrayRef<uint8_t> Data) {
+  if (hasError())
+    return; // Don't do anything if we have an error.
+  if (Data.empty())
+    return;
+  OS.write(reinterpret_cast<const char *>(Data.data()), Data.size());
+  if (!OS)
+    Error = llvm::make_error<llvm::StringError>("write failed",
+                                                llvm::inconvertibleErrorCode());
+}
+
+void FileWriter::writeCStr(llvm::StringRef Str) {
+  writeData(ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(Str.data()), 
+                              Str.size()));
+  if (!hasError())
+      writeU8(0); // NULL terminate the C string.
+}
+
+off_t FileWriter::tell() { 
+  if (hasError())
+    return -1;
+  return OS.tellp(); 
+}
+
+void FileWriter::seek(off_t Offset) {
+  if (hasError())
+    return;
+  OS.seekp(Offset);
+  if (!OS)
+    Error = llvm::make_error<llvm::StringError>("seek failed",
+                                                llvm::inconvertibleErrorCode());
+}
+
+void FileWriter::alignTo(size_t Align) {
+  off_t Offset = tell();
+  if (hasError())
+    return;
+  off_t AlignedOffset = (Offset + Align - 1) / Align * Align;
+  if (AlignedOffset == Offset)
+    return;
+  off_t PadCount = AlignedOffset - Offset;
+  std::string PadBytes(PadCount, '\0');
+  writeData(ArrayRef<uint8_t>(
+      reinterpret_cast<const uint8_t *>(PadBytes.data()), PadBytes.size()));
+}

lib/DebugInfo/GSYM/Range.cpp

 //===- Range.cpp ------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/GSYM/Range.h"
+#include "llvm/DebugInfo/GSYM/FileWriter.h"
+#include "llvm/Support/DataExtractor.h"
 #include <algorithm>
 #include <inttypes.h>
 
 using namespace llvm;
 using namespace gsym;
 
 
 void AddressRanges::insert(AddressRange Range) {
raw_ostream &llvm::gsym::operator<<(raw_ostream &OS, const AddressRanges &AR) {
   size_t Size = AR.size();
   for (size_t I = 0; I < Size; ++I) {
     if (I)
       OS << ' ';
     OS << AR[I];
   }
   return OS;
 }
+
+void AddressRange::encode(FileWriter &O, uint64_t BaseAddr) const {
+  assert(Start >= BaseAddr);
+  O.writeULEB(Start - BaseAddr);
+  O.writeULEB(size());
+}
+
+void AddressRange::decode(DataExtractor &Data, uint64_t BaseAddr,
+                          uint32_t &Offset) {
+  const uint64_t AddrOffset = Data.getULEB128(&Offset);
+  const uint64_t Size = Data.getULEB128(&Offset);
+  const uint64_t StartAddr = BaseAddr + AddrOffset;
+  Start = StartAddr;
+  End = StartAddr + Size;
+}
+
+void AddressRanges::encode(FileWriter &O, uint64_t BaseAddr) const {
+  O.writeULEB(Ranges.size());
+  if (Ranges.empty())
+    return;
+  for (auto Range : Ranges)
+    Range.encode(O, BaseAddr);
+}
+
+void AddressRanges::decode(DataExtractor &Data, uint64_t BaseAddr,
+                           uint32_t &Offset) {
+  clear();
+  uint64_t NumRanges = Data.getULEB128(&Offset);
+  if (NumRanges == 0)
+    return;
+  Ranges.resize(NumRanges);
+  for (auto &Range : Ranges)
+    Range.decode(Data, BaseAddr, Offset);
+}

unittests/DebugInfo/GSYM/GSYMTest.cpp

 //===- llvm/unittest/DebugInfo/GSYMTest.cpp -------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/DebugInfo/GSYM/FileEntry.h"
+#include "llvm/DebugInfo/GSYM/FileWriter.h"
 #include "llvm/DebugInfo/GSYM/FunctionInfo.h"
 #include "llvm/DebugInfo/GSYM/InlineInfo.h"
 #include "llvm/DebugInfo/GSYM/Range.h"
 #include "llvm/DebugInfo/GSYM/StringTable.h"
+#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Testing/Support/Error.h"
 
 #include "gtest/gtest.h"
 #include <string>
 
 using namespace llvm;
 using namespace gsym;
 
TEST(GSYMTest, TestStringTable) {
   EXPECT_EQ(StrTab.getString(1), "Hello");
   EXPECT_EQ(StrTab.getString(7), "World");
   EXPECT_EQ(StrTab.getString(8), "orld");
   // Test pointing to last NULL terminator gets empty string.
   EXPECT_EQ(StrTab.getString(12), "");
   // Test pointing to past end gets empty string.
   EXPECT_EQ(StrTab.getString(13), "");
 }
+
+static void TestFileWriterHelper(llvm::support::endianness ByteOrder) {
+  std::stringstream OutStrm;
+  FileWriter FW(OutStrm, ByteOrder);
+  const int64_t MinSLEB = INT64_MIN;
+  const int64_t MaxSLEB = INT64_MAX;
+  const uint64_t MinULEB = 0;
+  const uint64_t MaxULEB = UINT64_MAX;
+  const uint8_t U8 = 0x10;
+  const uint16_t U16 = 0x1122;
+  const uint32_t U32 = 0x12345678;
+  const uint64_t U64 = 0x33445566778899aa;
+  const char *Hello = "hello";
+  FW.writeU8(U8);
+  FW.writeU16(U16);
+  FW.writeU32(U32);
+  FW.writeU64(U64);
+  FW.alignTo(16);
+  const off_t FixupOffset = FW.tell();
+  FW.writeU32(0);
+  FW.writeUnsigned(U8, sizeof(U8));
+  FW.writeUnsigned(U16, sizeof(U16));
+  FW.writeUnsigned(U32, sizeof(U32));
+  FW.writeUnsigned(U64, sizeof(U64));
+  FW.writeSLEB(MinSLEB);
+  FW.writeSLEB(MaxSLEB);
+  FW.writeULEB(MinULEB);
+  FW.writeULEB(MaxULEB);
+  FW.writeCStr(Hello);
+  // Test Seek, Tell using Fixup32.
+  FW.fixup32(U32, FixupOffset);
+
+  // Make sure the FileWriter has no errors
+  EXPECT_FALSE(FW.takeError());
+
+  std::string Bytes(OutStrm.str());
+  uint8_t AddressSize = 4;
+  DataExtractor Data(Bytes, ByteOrder == llvm::support::little, AddressSize);
+  uint32_t Offset = 0;
+  EXPECT_EQ(Data.getU8(&Offset), U8);
+  EXPECT_EQ(Data.getU16(&Offset), U16);
+  EXPECT_EQ(Data.getU32(&Offset), U32);
+  EXPECT_EQ(Data.getU64(&Offset), U64);
+  Offset = alignTo(Offset, 16);
+  EXPECT_EQ(Data.getU32(&Offset), U32);
+  EXPECT_EQ(Data.getU8(&Offset), U8);
+  EXPECT_EQ(Data.getU16(&Offset), U16);
+  EXPECT_EQ(Data.getU32(&Offset), U32);
+  EXPECT_EQ(Data.getU64(&Offset), U64);
+  EXPECT_EQ(Data.getSLEB128(&Offset), MinSLEB);
+  EXPECT_EQ(Data.getSLEB128(&Offset), MaxSLEB);
+  EXPECT_EQ(Data.getULEB128(&Offset), MinULEB);
+  EXPECT_EQ(Data.getULEB128(&Offset), MaxULEB);
+  EXPECT_EQ(Data.getCStrRef(&Offset), StringRef(Hello));
+
+  // Test some things that cause the FileWriter to have an error
+  FW.writeUnsigned(U64, 3);
+  auto Error = FW.takeError();
+  handleAllErrors(std::move(Error),
+      [&](std::unique_ptr<ErrorInfoBase> EIB) {
+        EXPECT_EQ(EIB->message(),
+                  "unsupported byte size in FileWriter::writeUnsigned");
+      });
+}
+
+TEST(GSYMTest, TestFileWriter) {
+  TestFileWriterHelper(llvm::support::little);
+  TestFileWriterHelper(llvm::support::big);
+}
+
+TEST(GSYMTest, TestAddressRangeEncodeDecode) {
+  // Test encoding and decoding AddressRange objects. AddressRange objects
+  // are always stored as offsets from the a base address. The base address
+  // is the FunctionInfo's base address for function level ranges, and is
+  // the base address of the parent range for subranges.
+  std::stringstream OutStrm;
+  const auto ByteOrder = llvm::support::endian::system_endianness();
+  FileWriter FW(OutStrm, ByteOrder);
+  const uint64_t BaseAddr = 0x1000;
+  const AddressRange Range1(0x1000, 0x1010);
+  const AddressRange Range2(0x1020, 0x1030);
+  Range1.encode(FW, BaseAddr);
+  Range2.encode(FW, BaseAddr);
+  // Make sure the FileWriter has no errors
+  EXPECT_FALSE(FW.takeError());
+  std::string Bytes(OutStrm.str());
+  uint8_t AddressSize = 4;
+  DataExtractor Data(Bytes, ByteOrder == llvm::support::little, AddressSize);
+
+  AddressRange DecodedRange1, DecodedRange2;
+  uint32_t Offset = 0;
+  DecodedRange1.decode(Data, BaseAddr, Offset);
+  DecodedRange2.decode(Data, BaseAddr, Offset);
+  EXPECT_EQ(Range1, DecodedRange1);
+  EXPECT_EQ(Range2, DecodedRange2);
+}
+
+static void TestAddressRangeEncodeDecodeHelper(const AddressRanges &Ranges,
+                                               const uint64_t BaseAddr) {
+  std::stringstream OutStrm;
+  const auto ByteOrder = llvm::support::endian::system_endianness();
+  FileWriter FW(OutStrm, ByteOrder);
+  Ranges.encode(FW, BaseAddr);
+  // Make sure the FileWriter has no errors
+  EXPECT_FALSE(FW.takeError());
+
+  std::string Bytes(OutStrm.str());
+  uint8_t AddressSize = 4;
+  DataExtractor Data(Bytes, ByteOrder == llvm::support::little, AddressSize);
+
+  AddressRanges DecodedRanges;
+  uint32_t Offset = 0;
+  DecodedRanges.decode(Data, BaseAddr, Offset);
+  EXPECT_EQ(Ranges, DecodedRanges);
+}
+
+TEST(GSYMTest, TestAddressRangesEncodeDecode) {
+  // Test encoding and decoding AddressRanges. AddressRanges objects contain
+  // ranges that are stored as offsets from the a base address. The base address
+  // is the FunctionInfo's base address for function level ranges, and is the
+  // base address of the parent range for subranges.
+  const uint64_t BaseAddr = 0x1000;
+
+  // Test encoding and decoding with no ranges.
+  AddressRanges Ranges;
+  TestAddressRangeEncodeDecodeHelper(Ranges, BaseAddr);
+
+  // Test encoding and decoding with 1 range.
+  Ranges.insert(AddressRange(0x1000, 0x1010));
+  TestAddressRangeEncodeDecodeHelper(Ranges, BaseAddr);
+
+  // Test encoding and decoding with multiple ranges.
+  Ranges.insert(AddressRange(0x1020, 0x1030));
+  Ranges.insert(AddressRange(0x1050, 0x1070));
+  TestAddressRangeEncodeDecodeHelper(Ranges, BaseAddr);
+}