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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
