[Lldb-commits] [lldb] [lldb] Don't create functions for DWARF dead-code tombstones (PR #205691)

[Pavel Labath via lldb-commits]

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