[PATCH] D53379: GSYM symbolication format

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: include/llvm/DebugInfo/GSYM/GsymReader.h:46
+struct Header {
+  uint32_t Magic;
+  uint16_t Version;
----------------
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > With the switch to using BinaryStreamReader this might just work now if I understand the class directly. I switched over to decoding ArrayRefs for AddrOffsets, AddrInfoOffsets and FileEntry in the GsymReader. So this format can be any endian if needed. BinaryStreamReader would make a copy and swap them right?
> > I'm not sure I understand the question about making a copy and swapping.  `BinaryStreamReader` never makes a copy of anything (that's one of the nice things about it actually).  So you'd say something like:
> > 
> > ```
> > const Header *Header = nullptr;
> > BinaryStreamReader Reader(Stream);
> > Reader.readObject(Header);
> > ```
> > 
> > Note that header was never declared as an object to begin with, just a pointer.  So no copy ever happened.  All that happened was it changed the value of your pointer to point into the buffer.
> > 
> > Does that answer your question?
> I was commenting on the ArrayRef stuff I used when decoding an array of integers for the AddrOffsets, AddrInfoOffsets and FileEntry in GsymReader::init(...). I know pointers won't ever have their contents swapped. It would be easy to unswap the header if needed. I mostly worry about the array of address offsets and that is the only performance specific bottleneck if things are byte reversed.
That said, I think worrying about performance in byte-reversed scenarios is kind of a YAGNI concern.  Unless you're specifically planning to support big endian, we can address it if and when it comes.

That said though, reading the `ArrayRef` doesn't make a copy either.  It's just like all the other methods in that all it's going to do is `reinterpret_cast<>` the bytes.  So if you ask for an `ArrayRef<uint32_t>` it's just going to cast the bytes to a `uint32_t*` and put those into an `ArrayRef<>`.  But it's not going to copy anything.

If the reason you're asking is because `BinaryStreamReader` takes an endianness value to its constructor, that value is only used when you call the `readInteger` methods.  If you never call `readInteger()`, the endianness value you pass to the constructor is never used for anything.


================
Comment at: include/llvm/DebugInfo/GSYM/GsymReader.h:119
+  std::unique_ptr<MemoryBuffer> MemBuffer;
+  mutable BinaryStreamReader GSYMData;
+  const Header *GSYMHeader = nullptr;
----------------
Why does this need to be mutable?


https://reviews.llvm.org/D53379