[PATCH] D84825: [ELF] Change tombstone value -1 to 0

Thu Aug 6 01:04:06 PDT 2020

jhenderson added a comment.

TL;DR:

I think we should change to BFD semantics everywhere except .debug_loc and possibly .debug_ranges, because the BFD behaviour in the former is broken (and the behaviour in the latter is no different semantically to the LLD behaviour). I think we should stick with the current LLD behaviour (-2) for .debug_loc, because there's no benefit of 1 versus -2. The same argument applies for .debug_ranges.

---

Chiming in here, due to familiarity with the area, although I have to put the caveat that at Sony we want the on-by-default behaviour, since that replicates what our downstream version did before the change landed.

I agree with changing to BFD semantics as a reasonable step for now, for all sections, with the possible exception of .debug_ranges and definitely not for .debug_loc. The reason I except the latter two is because of the semantics involved. .debug_ranges and .debug_loc consist of a series of address pairs (with extra location information for .debug_loc), where each pair represents an address range, and where a (0, 0) pair represents an end-of-list. In the case of tombstombing, we cannot use 0 in .debug_ranges and .debug_loc because that will result in (0, 0) pairs in the middle of the list, causing premature termination of the list. However, we can use ANY other single value (1, 2, 123456, -2 etc) except -1 which also has a special meaning, because semantically, those values all result in an empty range (-2, -2)/(1, 1) etc. There is no need for special handling within consumers for any given value in this case (or at least there shouldn't be), and thus the specific BFD-identical value for .debug_ranges shouldn't matter.

Leaving a .debug_loc without a "tombstone" is actually a bug, since it could lead to premature list termination (a (0, 0) range - the BFD behaviour), or invalid debug data (a .debug_loc saying a range is (0, size of function)). It's possible this is partially handled by consumers, but certainly the former cannot be, since it is indistiguishable from a terminator. Picking any other empty range would resolve this issue. Since there's no prior (correct) art for this section, I don't think it matters what the value is, but even if there were, it shouldn't matter because consumers shouldn't be handling an empty range based on the specific value specified. They're all just empty ranges.

In D84825#2195626 <https://reviews.llvm.org/D84825#2195626>, @psmith wrote:

> There is a lot of info here, just summarising to confirm I've understood:
> BFD
>
> | Section                   | Tombstone |
> | .debug_loc, .debug_ranges | 1         |
> | .debug_*                  | 0         |
> |

This is incorrect - BFD patches .debug_loc to (0, 0), even if there is an addend, resulting in premature termination.

> I agree with MaskRay that we should move the discussion of a command line flag to another patch. It is difficult to follow the discussion here. I also think that the vast majority of users will never touch a command line option unless their program exhibits a problem with a debug processing tool. Getting the default behaviour changed is the most important thing.

I agree with this point. Let's not combine the two issues, even if they are related.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84825/new/

https://reviews.llvm.org/D84825