[Lldb-commits] [PATCH] D90598: [lldb/DWARF] Fix implementation of DW_OP_convert

Tue Nov 3 01:14:16 PST 2020

labath added a comment.

In D90598#2369590 <https://reviews.llvm.org/D90598#2369590>, @JDevlieghere wrote:

> In D90598#2368826 <https://reviews.llvm.org/D90598#2368826>, @dblaikie wrote:
>
>> That's my understanding. It means I'm not sure (though I'm not the expert here - certainly leave final decisions up to @aprantl and @JDevlieghere ) how this will be fixed, since lldb will presumably still need to be able to parse/understand old dsyms using the incorrect absolute offsets. (we had a similar problem once with DW_OP_bit_piece, where it was implemented incorrectly in both LLVM and lldb and I'm not sure how the backwards compatibility story was addressed there).
>
> I think @aprantl has proposed this in the past, but we should make `dsymutil` convert `DW_OP_convert` to `DW_OP_LLVM_convert` in its output.

How much space are we talking about saving here? A single DW_TAG_base_type DIE is like 4--8 bytes long (depending of whether it contains the name field, and how its encoded, etc.). If every compile unit needed a dozen of these entries, that's still like under 100 bytes per CU. Is that too much? Could we just put these entries into every CU that needs them? I'd expect this to be negligible compared to the rest of DWARF...

If the space savings are important enough, then staking out a new dwarf opcode seems like the best solution. However, since DWARF Linker is also coming to other platforms and those platforms may have consumers which may not understand llvm extensions, I think we may have to implement both solutions anyway. Also, to support non-macho use cases, I think the operand to this extension should not be uleb-encoded (so that it can be relocated by a linker), which can make it larger than usual.

Or.. here's a crazy idea. Do all compile units produced by dsymutil share the same abbreviation table? (If not, why not?) We could encode the DW_TAG_base_types "into" the abbreviation table via DW_FORM_implicit_const. Then the DW_TAG_base_types would be just `sizeof(uleb) ≈ 2` (plus maybe the size of a string form, if they have a name).

> I also don't have a good plan for backward compatibility. If there was a way we could "know" that the DWARF came from a dSYM we could continue to support the old behavior knowing that it has always been wrong, but I'm not aware of anything like that in LLDB and I'm also not sure that's something I'd like to have in the first place...

Detecting dSYM files should not be that hard. `triple.isMachO() && !symbol_file_dwarf->GetDebugMapSymfile()` should be a pretty good approximation. It won't handle case the case of opening .o files directly, but that's only useful for tests I believe, and we could throw in an additional condition for that if needed. If we want to have backward compatibility, I think we're going to have to have something like that at least for a transitional period. The question is how to ensure that this code can be removed eventually. One possibility would be for dsymutil to signal that it has this fixed via some cu-level attribute (DW_AT_LLVM_convert_is_fixed) --  after a certain time, we can remove the fallback, and assume the correct behavior everywhere...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90598