[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Greg Clayton via lldb-commits]

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