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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

In D63828#1567764 <https://reviews.llvm.org/D63828#1567764>, @MaskRay wrote:

> In D63828#1567323 <https://reviews.llvm.org/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.


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

https://reviews.llvm.org/D63828