clayborg wrote:
> My previous message reflected my confusion about the usefulness about the
> DW_IDX_cu+DW_IDX_tu combo. Let me try to start over.
>
> The thing I don't like about this patch is the duplication between
> `getForeignTUSkeletonCUOffset` (the new function) and `getCUOffset` (the
> existing function). Since the duplication is motivated by undesirable
> behavior (result) of the existing function in some cases, this got me
> thinking if there's a way to define a single function in a way that will be
> usable in all scenarios (and still have a consistent easy-to-understand
> definition).
>
> To see if that's possible, let's consider the possible cases (I'm putting
> cases with DW_IDX_tu first, as those are the interesting ones here):
>
> 1. A single-CU index, DW_IDX_cu not present, DW_IDX_tu present
> 2. A multi-CU index, DW_IDX_cu and DW_IDX_tu present
>
> These two definitely are the most common (of those including type units)
> ones, but we can also have:
>
> 3. A single-CU index, DW_IDX_cu and DW_IDX_tu present
> 4. A multi-CU index, DW_IDX_cu not present, DW_IDX_tu present
>
> For completeness, we can also have the cases without type units. I will
> include only include one here, as I think the others are obvious:
>
> 5. A single-CU index, DW_IDX_cu and DW_IDX_tu **not** present
>
> Now that we have that, what are the possible `getCUOffset`? I can think of
> three:
>
> a) "return the value of DW_IDX_cu. Period." If an explicit value is not
> present return nothing. I don't think anyone wants this, but I'm mentioning
> it for completeness. With this definition, the function should return the cu
> offset only in the second and third cases (`NYYNN`)
> b) "return the DW_IDX_cu value, or the single CU from the index." Let's call
> this the "raw" version. With this definition, I think this function should
> return the cu offset in all cases except the fourth (that one is impossible
> to resolve): `YYYNY`
> c) "return the DW_IDX_cu value, only if the entry describes a DIE within that
> CU." Let's call this the "semantic" version. I believe a function with this
> definition should not return a CU offset only in the last case: `NNNNY`
>
> Now, the problem I see is that the current implementation of `getCUOffset` is
> **neither** of these options. If an entry has an (explicit) DW_IDX_cu, it
> will always return it (even if it also contains DW_IDX_tu). However, an
> implicit CU will only be returned if DW_IDX_tu is not present. I.e, this
> function does a `NYNNY`.
>
> Therefore, my proposal is to redefine the function to do (b), which
> essentially amounts to deleting these two lines:
>
> ```
> if (lookup(dwarf::DW_IDX_type_unit).has_value())
> return std::nullopt;
> ```
But we won't want a CU index back if we have a `DW_IDX_type_unit` that is a
local type unit. Makes no sense to return a CUOffset from such entries. So
maybe we modify this be:
```
std::optional<uint64_t> DWARFDebugNames::Entry::getCUIndex() const {
if (std::optional<DWARFFormValue> Off = lookup(dwarf::DW_IDX_compile_unit))
return Off->getAsUnsignedConstant();
// In a per-CU index, the entries without a DW_IDX_compile_unit attribute
// implicitly refer to the single CU, but only if we don't have a
// DW_IDX_type_unit that is a foreign type unit.
if (IsForeignTypeUnit())
return std::nullopt;
if (NameIdx->getCUCount() == 1)
return 0;
return std::nullopt;
}
```
Then we implement a `bool IsForeignTypeUnit() const` that just checks if there
is a DW_IDX_type_unit whose index is a foreign type unit.
>
> Besides providing a consistent definition of the function (YMMV, if you can
> provide an explanation for the current behavior, I'd like to hear it), it
> would also enable you to reuse it in `getForeignTUSkeletonCUOffset`, as that
> is exactly the behavior you need there (although, at that point, I don't
> think `getForeignTUSkeletonCUOffset` needs to exist as a separate function,
> as it will be simple enough to inline into lldb).
That is fine with me as long as you agree with the above fix.
Let me know if the above fix to `DWARFDebugNames::Entry::getCUIndex()` makes
sense.
https://github.com/llvm/llvm-project/pull/87740
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
