[PATCH] D84825: [release/11.x only][ELF] Change tombstone value -1 to 0
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 4 23:23:13 PDT 2020
On Tue, Aug 4, 2020 at 11:22 PM Fangrui Song via Phabricator <
reviews at reviews.llvm.org> wrote:
> MaskRay added a comment.
>
> In D84825#2195418 <https://reviews.llvm.org/D84825#2195418>, @echristo
> wrote:
>
> > 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:
>
> My comment is about .debug_loc . I am under the impression that the
> agreement is that we can use -2 for .debug_loc but @dblaikie's comments
> seemed to suggest 1. I want to better understand why 1 works better.
>
> > 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.
>
> I hope there was more information-rich reply about what sections caused
> problems. As far as I can tell, .debug_line is currently the only problem.
> I don't necessarily object to change other .debug_*
> but I think it is unfair for me/the community to get another behavior
> change without more context.
>
>
At which point we are at an impasse and the default behavior applies.
Please revert completely back to the original behavior immediately.
Thanks.
-eric
> > 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 :)
>
> This discussion is not resolved and it is an important one. If @dblaikie
> wants an option, I think we should discuss it in another patch. Adding the
> option in this patch is not critical as far as I can tell.
>
>
> Repository:
> rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
> https://reviews.llvm.org/D84825/new/
>
> https://reviews.llvm.org/D84825
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200804/950bf2e9/attachment.html>
More information about the llvm-commits
mailing list