[PATCH] D84825: [release/11.x only][ELF] Change tombstone value -1 to 0

Wed Jul 29 13:58:53 PDT 2020

MaskRay added a comment.

In D84825#2183080 <https://reviews.llvm.org/D84825#2183080>, @dblaikie wrote:

> In D84825#2182964 <https://reviews.llvm.org/D84825#2182964>, @MaskRay wrote:
>
>> In D84825#2182896 <https://reviews.llvm.org/D84825#2182896>, @dblaikie wrote:
>>
>>> In D84825#2182878 <https://reviews.llvm.org/D84825#2182878>, @MaskRay wrote:
>>>
>>>> In D84825#2182460 <https://reviews.llvm.org/D84825#2182460>, @dblaikie wrote:
>>>>
>>>>> Please implement a clear flag for
>>>>>
>>>>> - BFD semantics
>>>>> - New (-1/-2) semantics
>>>>>
>>>>> And default to the BFD semantics for now.
>>>>>
>>>>> The change in semantics is not ready to ship by default - there are lots of DWARF consumers out there and lots of people don't update them in lock-step with their compiler.
>>>>
>>>> I don't know what the request is. The patch implemented:
>>>>
>>>> - .debug_ranges & .debug_loc: -2
>>>> - .debug_*: 0  (GNU ld semantics)
>>>>
>>>> If you meant full BFD semantics:
>>>>
>>>> - .debug_ranges: 1
>>>> - .debug_*: 0
>>>
>>> Yes, that's what I meant. (though probably applying the debug_ranges workaround to debug_loc as well would be suitable but not necessary, I think)
>>
>> Did you mean
>>
>>   - * .debug_ranges & .debug_loc: -2
>>   + * .debug_ranges & .debug_loc: 1
>>
>> ?
>> I think this has gone too far a step. We really need more evidence that 1 can make DWARF consumers happier.
>
> That's fairly easy - 0, 0 terminates a location list the same way it terminates a range list - llvm-dwarfdump, for instance, then tries to parse the following expression as the start of a new location list and errors. binutils objdump only dumps location lists that are referenced from debug_info, printint out "holes" when the location list terminates early.
>
> eg:
>
>   $ g++-tot loc.cpp -g -ffunction-sections -Wl,-gc-sections -O3 -fuse-ld=bfd
>   $ llvm-dwarfdump-tot a.out
>   .debug_loc contents:
>   0x00000000: 
>               (0x0040102000040402, 0x0040102000000000): 
>               (0x10209f3300020000, 0x1023000000000040): <decoding error> 00 00 00 00 02 00 37 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 04 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 33 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>               (0x000000009f370002, 0x0000000000000000): error: unexpected end of data at offset 0x78 while reading [0x76, 0x7e)
>   $ objdump -g a.out | grep -A50 debug_loc
>   objdump: Warning: Hole and overlap detection requires adjacent view lists and loclists.
>   objdump: Warning: Contents of the .debug_loc section (loaded from a.out):
>   There are 40 unused bytes at the end of section .debug_loc
>   
>       Offset   Begin            End              Expression
>   
>       00000000 v000000000000002 v000000000000004 location view pair
>       00000002 v000000000000004 v000000000000000 location view pair
>   
>       00000004 v000000000000002 v000000000000004 views at 00000000 for:
>                0000000000401020 0000000000401020 (DW_OP_lit3; DW_OP_stack_value)
>       00000018 v000000000000004 v000000000000000 views at 00000002 for:
>                0000000000401020 0000000000401023 (DW_OP_lit7; DW_OP_stack_value)
>       0000002c <End of list>
>   
>       0000003c v000000000000002 v000000000000004 location view pair
>       0000003e v000000000000004 v000000000000000 location view pair
>   
>       00000040 <End of list>
>
> (have to use GCC to produce the input here, since Clang's current use of base address specifiers in debug_loc doesn't trip over the issue - that doesn't diminish the problem, though)

Can you share `loc.cpp`? How does  `.debug_loc: 1` help while `.debug_loc: -2` doesn't work?

>>>> I also don't want to add another option like `--gnu-ld-debug-reloc-semantics` or similar.
>>>>
>>>> If you meant a shortcut to
>>>>
>>>> - -z 'dead-reloc-in-nonalloc=.debug_loc=0xfffffffffffffffe'
>>>> - -z 'dead-reloc-in-nonalloc=.debug_ranges=0xfffffffffffffffe'
>>>> - -z 'dead-reloc-in-nonalloc=.debug_*=0xffffffffffffffff'
>>>>
>>>> I don't that is necessary either. That option will be our compatibility burden. If would cause pain when we want to remove it in LLD 12 or 13.
>>>
>>> If we continue to support a broader/more configurable flag, I'm not sure how much support burden having a narrower flag would cause.
>>>
>>> Actually having the broad flag is concerning to me because it increases the support burden and surface area fo the linker and potentially creates far more diverse DWARF environment than existed before this change (when you could only get one of two semantics - bfds or gold/llds) making interoperability between DWARF consumers more difficult.
>>
>> `-z dead-reloc-in-nonalloc=` is a more general option. As you see, its name does not even mention `debug` but the `.debug_*` use cases are just a subset. I have mentioned one use case for non-.debug* in D83264 <https://reviews.llvm.org/D83264>.
>
> It's a nice hypothetical, but doesn't seem to be a pressing need for anyone right now. In any case, I'd still prefer that DWARF users opt in to one of two well defined semantics here, not be left to customize the semantics on a per-section basis. Both for being able to opt-in to the new semantics to test them before they become the default, and to be able to opt-out after the default changes. Both of those operations need to be clearly accessible without increased chance of user error and avoiding the chance of wider proliferation of variants DWARF consumers would need to be aware of.

I actually don't find a pressing need for the option with `gnu-ld` or `bfd` in its name: it will likely never be adopted by GNU ld. GNU ld's behaviors can change, it may patch `.debug_loc` to use 1 or -2 if your example above is a problem. Are we naming the option `--gnu-ld-2.35-compatible-dead-reloc-in-debug`? Users may have to dig up these long discussion threads to figure out what GNU ld uses. I think it is also important to be specific in these cases, what tombstone values are used. The documentation just says what values are supported and others are not recommended. That is all. That is practically everything we can do. Beyond that is entirely user problems. Users can shoot themselves by specifying strange values to --image-base & `-z max-page-size=`. They are on their own when doing this.

>> I admit that I committed D83264 <https://reviews.llvm.org/D83264> a bit too quickly, because I was receiving pressure from @thakis who urgently needed the option. I would usually wait longer for @grimar and @psmisth to comment (@psmith had expressed ok in his first comment but @grimar hadn't) Hope they are still happy with the option)
>>
>> About more diverse DWARF environment: I think the documentation can be improved to mention that fiddling with this needs to be very careful.  The debug info is admittedly important, but the option doesn't contribute too much to the already very wide spectrum we have to support: many linker options can already affected the linked image in a complicate interaction users should be very careful: --dynamic-list --export-dynamic-symbol --no-dynamic-linker --sysroot --version-script --no-rosegment -z separate-code --image-base.
>
> Not sure I follow how these are related to making this issue more user-friendly (& more importantly, more ecosystem friendly - avoiding proliferation of variants seems important as we're trying to get back from 2 variants with different tradeoffs and adding a 3rd with the hopes we'll all converge on one eventually, but adding the quite accessible possibility of many more seems counter to that goal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84825