labath wrote:
This is an interesting way to fix the problem. It kinda makes sense because
this is the part that was rejecting the "-1" tombstone in the 64-bit case.
However, it is also incomplete as it only applies to the DW_AT_low/high_pc
scenario. If the same values are passed through DW_AT_ranges, then they will
still slip through (AFAICT, it has no comparable checks). It's also unfortunate
as it's separated from the main tombstone check, which happens in
SymbolFileDWARF::ParseFunction.
You say lldb doesn't have a explicit tombstone check. That's kinda true, but
only if you include the "explicit" part. The reason we don't have a check like
the one you're adding is because there isn't a universal consensus on the
tombstoning process. The -1 thingy is an LLVM invention, and other linkers will
either use a zero or a value close to zero. This is why the check looks like
this:
```
if (range.valid() && range.LowPC < m_first_code_address)
```
`range.valid()` does a `low_pc <= high_pc` (regardless of where the range came
from) and `range.LowPC < m_first_code_address` checks for values that are too
low to be real. So I'd focus on that. I don't know whether the fix is to make
high_pc computation wrap around on UINT32_MAX for 32-bit arches (so that low <=
high fails as expected), or to add a high_pc>UINT32_MAX check for 32-bit
targets, but I think this is the best place do to that.
-----
The check that comes after this is also interesting, because it *looks like* it
tries to check that the address resolves to a valid section:
```
if (Address base_addr(range.LowPC, module_sp->GetSectionList());
base_addr.IsValid() && FixupAddress(base_addr))
```
which is exactly the thing you're trying to guard against in
`Block::GetRangeContainingAddress` (which I see you've still left in this
patch). It doesn't actually do that because IsValid is essentially another way
to check for -1 (LLDB_INVALID_ADDRESS). If that is what we want, then we really
need the check in GetRangeContainingAddress (and maybe elsewhere too), but I
wonder what would happen if we change that to `base_addr.IsSectionOffset()`.
Changing that on linux resulted in only one test failure and that was a
synthetic test (`SymbolFile/DWARF/gnu-ref-strp-alt.test`), which probably just
needs to be made more realistic. This would have the advantage of being able to
trust the function address more in downstream code.
https://github.com/llvm/llvm-project/pull/205691
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
