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

Tue Aug 4 23:12:44 PDT 2020

echristo added a comment.

In D84825#2195362 <https://reviews.llvm.org/D84825#2195362>, @MaskRay wrote:

> In D84825#2183197 <https://reviews.llvm.org/D84825#2183197>, @dblaikie wrote:
>
>> In D84825#2183130 <https://reviews.llvm.org/D84825#2183130>, @MaskRay wrote:
>>
>>> 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`?
>>
>> Oh, right, sorry:
>>
>>   __attribute__((noinline)) void f1() { }
>>   void f2() {
>>     int i = 3;
>>     f1();
>>     i = 7;
>>     f1();
>>   }
>>   int main() {
>>     int i = 3;
>>     f1();
>>     i = 7;
>>     f1();
>>   }
>>
>> 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.
>
> So how is BFD's choice for .debug_loc (1) better than -2? Additional warning `objdump: Warning: Hole and overlap detection requires adjacent view lists and loclists. objdump: Warning: There are 40 unused bytes at the end of section .debug_loc` and unneeded fffffffffffffffe ranges cannot be filtered out?

This isn't a matter of which long term design we want to go to - it's a matter of making that transition easy for our users. We need to be able to better plan this transition. The amount of chaos in multiple places has shown that. You've also had numerous requests to make this behavior change - by default if we can't come to agreement we revert back to before the change which would mean you have two options at this point:

a) Make the change to the bfd behavior
b) Revert back to the original behavior

Please do one of these two. I was under the impression, especially after Petr's email, that this had already happened.

My personal preference is for the bfd behavior as that's widespread enough that it solves some of the problems we've seen with the tables and gives us a nice place to continue the transition.

>> I don't have strong opinions about the names of the flags - "legacy" seems suitable for a general term without saying "bfd" in particular. -Wl,-no-legacy-dwarf-tombstones would let people opt-in to the behavior before it's the default and -Wl,-legacy-dwarf-tombstones to opt-out once the default has changed.
>
> If you are so strong about non-customization of dead relocations in .debug_* => if something like `-z dead-reloc-in-nonalloc=.debug_loc=` appears, we can validate that the value is from a fixed set. For example, for .debug_line, it can be either 0 or -1. Other values will get a warning.
>
> We can still avoid a temporary option.

We can discuss this after in a separate code review. Naming things is hard :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84825