[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

> Please undo all renaming from offset to file_offset.

I am preparing now a split of this patch for the renaming part + the "real" part, sorry I did not realize it is mostly about the renaming which is not worth your detailed review.

There are two different types of offsets:

| `UniqOffset` | One DIE from a `DW_TAG_partial_unit` gets two different `UniqOffset` depending from which `DW_TAG_compile_unit` it was included |
| `FileOffset` | One DIE from a `DW_TAG_partial_unit` has only one `FileOffset` - where one can read it from `get_debug_info_data()`             |

Most of LLDB works with `UniqOffset` so I kept the original `dw_offset_t offset` type+name for it. When LLDB works with `FileOffset` I call it `dw_offset_t file_offset`. I really discourage calling these two different offset types the same as it is a bug (although affecting only DWZ files) if there is missing a proper conversion function between these two types.

I found overengineered to make a separate type-safe against accidental exchange wrapper class  for the `FileOffset' but I can if you prefer that over a simple renaming of all the variables/parameters.

https://reviews.llvm.org/D40474 introduces `DWARFFileOffset` which makes it type-safe against accidental exchange with `dw_offset_t` but that type includes also `bool is_dwz` whether it is in DWZ Common File or in the main symbol file which makes it excessive for the places where currently I used `dw_offset_t file_offset` because the file itself is known there.

> The "offset" you get from a  DIE should always be the absolute .debug_info offset. No need to say file_offset.

We never talk about CU-relative offset.  I made remapping in `DWARFDebugInfoEntry`:

| `dw_offset_t GetOffset()` renamed to | `FileOffset`-kind `dw_offset_t GetFileOffset()`                       |
| new                                  | `UniqOffset`-kind `dw_offset_t GetOffset(const DWARFCompileUnit *cu)` |



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:207-226
+  dw_offset_t GetFileOffset() const { return m_data->m_file_offset; }
+  dw_offset_t FileOffsetToUniqOffset(dw_offset_t file) const {
+    return ThisCUFileOffsetToUniq(file);
+  }
+  static dw_offset_t ThisCUFileOffsetToUniq_nocheck(dw_offset_t file) {
+    return file;
+  }
----------------
clayborg wrote:
> Not a fan of these names. Can't these just be something like:
> 
> ```
> dw_offset_t GeCUtRelativeOffset(); // CU relative offset
> dw_offset_t GetOffset(); // Always absolute .debug_info offset
> ```
> 
> ThisCUUniqToFileOffset seems a bit unclear.
What should be `dw_offset_t GeCUtRelativeOffset(); // CU relative offset`?
This patchset never deals with offsets relative to the CU start (and LLDB never uses such offsets, `DWARFCompileUnit::ExtractDIEsIfNeeded` stores their `.debug_info`-relative offsets and `DWARFFormValue::Reference()` also converts everything into `.debug_info`-relative offsets.
| `UniqOffset` -> `FileOffset` | `DWARFDebugInfo::GetCompileUnit`{,`ContainingDIEOffset`} | slow `m_compile_units` bisecting |
| `UniqOffset` -> `FileOffset` | `DWARFCompileUnit::ThisCUUniqToFileOffset` | fast but the CU must match |
| `FileOffset` -> `UniqOffset` | `DWARFCompileUnit::ThisCUFileOffsetToUniq` | fast, this functionality requires to use matching CU |
`UniqOffset` is in the most simple case without using DWZ the same as `FileOffset`.  If DWZ is used then see the mapping in Summary above.
The methods cannot be called `Get' as they convert their parameter value, they are not getters.



https://reviews.llvm.org/D40467