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