[PATCH] D53379: GSYM symbolication format

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: lib/DebugInfo/GSYM/README.md:59
+### Address Data Offsets Table
+The address data offsets table immediately follows the address table and consists of `Header.num_addrs` 32 bit file offsets: one for each address in the address table. The offsets in this table are the absolute file offset to the address data for each address in the address table. Keeping this data separate from the address table helps to reduce the number of pages that are touched when address lookups occur on a GSYM file.
+
----------------
lemo wrote:
> clayborg wrote:
> > lemo wrote:
> > > Why absolute offsets as opposed to relative offsets into the data section? 
> > > 1. at very least it makes it easier to manipulate the file format
> > > 2. it may also enable short offsets?
> > > 3. also consistent with strings offsets
> > We need to be able to binary search this table for your address. If we use relative offsets, then we can't do that. The idea is to mmap this file and use the data as is with minimal setup.
> Sorry if I wasn't clear, I was talking about the absolute file offsets: 
> 
> > The offsets in this table are the absolute file offset to the address data for each address in the address table.
> 
> These are just file pointers to the address data, so relative vs. absolute has nothing to do with the binary search in the address table, right? Also, absolute 32bit file offsets could be limiting simply because they can only address 4Gb (as section-relative offsets this is less of a concern)
Actually these are section relative offsets already. They are absolute file offsets if the GSYM is stand alone, but if the GSYM is in a section, it will be section relative. Sorry for the confusion. I will update the documentation to say this more clearly.


================
Comment at: lib/DebugInfo/GSYM/README.md:83
+
+### String Table
+The string table follows the file table in stand alone GSYM files and contains all strings for everything contained in the GSYM file. Any string data should be added to the string table and any references to strings inside GSYM information must be stored as 32 bit string table offsets into this string table.
----------------
lemo wrote:
> clayborg wrote:
> > lemo wrote:
> > > Have you considered sorting the strings + prefix compression? It's an easy way to compress the strings and would avoid the need for special hasing things like directory / filename split in the FileInfo
> > I haven't really done much optimization on paths other that split them into directory and filename so file entries can share the strings. One thing we could do is allow strings to be specified in the string table with a length for the file table. That way we could have a long path: /a/b/c/d and refer to "/a", "/a/b", "/a/b/c" and "/a/b/c/d" using the same string. I am open to ideas here. I kept it simple to start with.
> What I had in mind is to simply provision for prefix compression - for example every string having an optional link to its prefix (which can also be prefix compressed). What do you think?
I am open to ideas on this. For paths I thought about this idea of chaining but directory names are often short and using 4 bytes for the chain might not make for big wins in gain and adds complexity. I am happy to discuss any string table optimizations that you have in mind though.


================
Comment at: lib/DebugInfo/GSYM/README.md:84
+### String Table
+The string table follows the file table in stand alone GSYM files and contains all strings for everything contained in the GSYM file. Any string data should be added to the string table and any references to strings inside GSYM information must be stored as 32 bit string table offsets into this string table.
+
----------------
lemo wrote:
> Can you please document the exact format used to store strings? (even if it's just to note that it uses the same format as .debug_str for example)
Will do


Repository:
  rL LLVM

https://reviews.llvm.org/D53379