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

[Pavel Labath via lldb-commits]

labath 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;
```

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).

Changing the definition of course requires going through the call sites and making sure they are okay with that definition. I counted a total of three, one in llvm and two in lldb. The call in llvm (llvm-dwarfdump.cpp) is broken for type units, both before and after this change. Maybe the same goes for one of the calls in lldb (DebugNamesDWARFIndex::GetGlobalVariables -- I'm not sure if you can have global (static class member) variables in a type unit). The last one is definitely broken (and the test suite attests that, but fixing it is trivial: see https://github.com/llvm/llvm-project/commit/c5c2a9d61132c81756267694dd7af52d0a1121d9. I'll also note that this code was also broken for the case (2) -- a multi CU-index with the accelerator entry referring to a DIE in a (local) type unit, and that that patch fixes that.

https://github.com/llvm/llvm-project/pull/87740