[ELF] Change tombstone values to (.debug_ranges/.debug_loc) 1 and (other .debug_*) 0

[MaskRay]

Summary

tl;dr See D81784 for the 'tombstone value' concept. This patch changes our behavior to be almost the same as GNU ld (except that we also use 1 for .debug_loc):

• .debug_ranges & .debug_loc: 1 (LLD<11: 0+addend; GNU ld uses 1 for .debug_ranges)
• .debug_*: 0 (LLD<11: 0+addend; GNU ld uses 0; future LLD: -1)

We make the tweaks because:

1. The new tombstone is novel and needs more time to be adopted by consumers before it's the default.
2. The old (gold) strategy had problems with zero-length functions - so rather than going back that, we're going to the GNU ld strategy which doesn't have that problem.
3. One slight tweak to (2) is to apply the .debug_ranges workaround to .debug_loc for the same reasons it applies to debug_ranges - to avoid terminating lists early.

---

http://lists.llvm.org/pipermail/llvm-dev/2020-July/143482.html

The tombstone value -1 in .debug_line caused problems to lldb (fixed by D83957;
will be included in 11.0.0) and breakpad (fixed by
https://crrev.com/c/2321300). It may potentially affects other DWARF consumers.

For .debug_ranges & .debug_loc: 1, an argument preferring 1 (GNU ld for .debug_ranges) over -2 is that:

{-1, -2}    <<< base address selection entry
{0, length} <<< address range

may create a situation where low_pc is greater than high_pc. So we use
1, the GNU ld behavior for .debug_ranges

For other .debug_* sections, there haven't been many reports. One issue is that
bloaty (src/dwarf.cc) can incorrectly count address ranges in .debug_ranges . To
reduce similar disruption, this patch changes the tombstone values to be similar to GNU ld.

This does mean another behavior change to the default trunk behavior. Sorry
about it. The default trunk behavior will be similar to release/11.x while we work on a transition plan for LLD users.

[hans]

Thanks! Can you add a note in docs/ReleaseNotes.rst explaining that this is expected to change in 12, what that means, what flags one should use to try it, and what problems to look out for?

[dblaikie]

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.

[MaskRay]

Add a note to the "Breaking changes" section of lld/docs/ReleaseNotes.rst

[MaskRay]

Update release note

[MaskRay]

In 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

Sorry, I don't think that is necessary. 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. The -z dead-reloc-in-nonalloc option is generic and can potentially be useful for other non .debug_* sections as well. It is also in a form that GNU ld can hopefully adopt if it can find its uses for other non .debug_* sections.

[dblaikie]

In D84825#2182878, @MaskRay wrote:

In 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)

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.

[MaskRay]

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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.

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. I admit that I committed 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.

[dblaikie]

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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)

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.

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 admit that I committed 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).

[MaskRay]

In D84825#2183080, @dblaikie wrote:

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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.

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 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).

[dblaikie]

In D84825#2183130, @MaskRay wrote:

In D84825#2183080, @dblaikie wrote:

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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();
}

How does .debug_loc: 1 help while .debug_loc: -2 doesn't work?

Sorry, it doesn't. This was under the "default to BFD semantics with this one modification (treat debug_loc the same as debug_ranges)" - 1 helps here 0 is problematic. Extending BFD's existing special case to debug_ranges seems like a relatively low-risk change that avoids a minor regression to LLD's semantics when switching from gold's semantics to bfd's semantics.

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.

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.

Reducing the chance users can shoot themselves in the foot seems valuable. And reducing the chance that they'll diverge is my greater concern (similar to why Clang has some /very/ big warnings on the flags needed to turn on zero-initialization by default for C++ because we really don't want people to take a long-term dependence on this).

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.

I admit that I committed 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).

[MaskRay]

In D84825#2183197, @dblaikie wrote:

In D84825#2183130, @MaskRay wrote:

In D84825#2183080, @dblaikie wrote:

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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?

-    00000040 <End of list>
+    00000040 v000000000000002 v000000000000004 views at 0000003c for:
+             fffffffffffffffe fffffffffffffffe (DW_OP_lit3; DW_OP_stack_value)
+    00000054 v000000000000004 v000000000000000 views at 0000003e for:
+             fffffffffffffffe fffffffffffffffe (DW_OP_lit7; DW_OP_stack_value) (start > end)
+    00000068 <End of list>

How does .debug_loc: 1 help while .debug_loc: -2 doesn't work?

Sorry, it doesn't. This was under the "default to BFD semantics with this one modification (treat debug_loc the same as debug_ranges)" - 1 helps here 0 is problematic. Extending BFD's existing special case to debug_ranges seems like a relatively low-risk change that avoids a minor regression to LLD's semantics when switching from gold's semantics to bfd's semantics.

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.

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.

Reducing the chance users can shoot themselves in the foot seems valuable. And reducing the chance that they'll diverge is my greater concern (similar to why Clang has some /very/ big warnings on the flags needed to turn on zero-initialization by default for C++ because we really don't want people to take a long-term dependence on this).

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.

I admit that I committed 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).

[echristo]

In D84825#2195362, @MaskRay wrote:

In D84825#2183197, @dblaikie wrote:

In D84825#2183130, @MaskRay wrote:

In D84825#2183080, @dblaikie wrote:

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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 :)

[MaskRay]

In D84825#2195418, @echristo wrote:

In D84825#2195362, @MaskRay wrote:

In D84825#2183197, @dblaikie wrote:

In D84825#2183130, @MaskRay wrote:

In D84825#2183080, @dblaikie wrote:

In D84825#2182964, @MaskRay wrote:

In D84825#2182896, @dblaikie wrote:

In D84825#2182878, @MaskRay wrote:

In 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.

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.

[MaskRay]

At which point we are at an impasse and the default behavior applies. Please revert completely back to the original behavior immediately.

Thanks.

With respect I think the "request for changes" blocked the change and I am not inclined to agree we would otherwise be at an impasse without the "request for changes".
I would likely get an "Accept Revision" or a just textual "LGTM", even if there is a conditional request that ".debug_* should use 0 as well, like GNU ld". I have mentioned that my position has been weakened. (I made the reply after 5 days, not my usual style as you can tell, because the related threads have caused so much stress/pain to me. I would otherwise not be able to focus on real work. In some sense, I feel that I am fighting a really meaningless battle, even the original proposers have not said something on this patch - Why do I still care about the resolution so much? The all things make me feel I can't really make a fair decision on this patch. I regret that I was involved in the original implementation. I would definitely better if I were just a reviewer)

I sent the patch instead of reverting 3 or 4 intertwined dependent patches because I think reverting will just make the situation more confusing. I have experienced the review processes of all relevant patches and have read all the context,
so I very responsibly state again that "a simple revert of 3 or 4 dependent patches will just make the situation more confusing".

I will very appreciate feedback from other people, not my colleague, especially LLD contributors such as @psmith @grimar @jhenderson, whether we really need a flag in this patch or can we simply defer it to another thread.

Let me restate my weakened position:

• .debug_loc|.debug_ranges: -2 (if @dblaikie can give me clearer example that .debug_loc should really use 1, 1 is inferior to -2, I'd like to follow)
• .debug_*: 0
• no command line option. Defer it to another thread.

[psmith]

My apologies for not commenting earlier. I haven't had a lot of spare time and I only have a weak opinion, certainly far weaker than the other commenters.

There is a lot of info here, just summarising to confirm I've understood:
LLD with this patch:

Section	Tombstone
.debug_loc, .debug_ranges	-2
.debug_*	0

BFD

Section	Tombstone
.debug_loc, .debug_ranges	1
.debug_*	0

Future desired values for LLD

Section	Tombstone
.debug_loc, .debug_ranges	-2
.debug_*	-1

LLD currently has, for better or worse, a flexible way of setting the tombstone value for different section types.

As I understand it the desired behaviour from reviewers is the BFD behaviour by default and a coarser grained command line option. With the argument for being that LLD could conform to any dwarf consumer's preference and the argument against that dwarf consumers would prefer not to handle users arbitrary choices of tombstones.

My weak opinion:
I'm not qualified to say whether -2 is better or worse than 1 for the tombstone value. I'll have to leave that to the debug experts. In the general case I think that in an ecosystem where people mix compilers, linkers and debuggers, consistency between tools is often more important than the best solution. I'd tend towards staying compatible with BFD without a good case for doing something different, but as I'm not a Dwarf expert I don't think my opinion carries much weight here.

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.

In my experience from embedded systems it is usually something like the vector table that goes at address 0, the most important thing I've seen users complain about is source file locations as the vector table is usually written in assembly and there isn't much (symbols/types) to debug.

[echristo]

In D84825#2195429, @MaskRay wrote:

At which point we are at an impasse and the default behavior applies. Please revert completely back to the original behavior immediately.

Thanks.

With respect I think the "request for changes" blocked the change and I am not inclined to agree we would otherwise be at an impasse without the "request for changes".
I would likely get an "Accept Revision" or a just textual "LGTM", even if there is a conditional request that ".debug_* should use 0 as well, like GNU ld". I have mentioned that my position has been weakened. (I made the reply after 5 days, not my usual style as you can tell, because the related threads have caused so much stress/pain to me. I would otherwise not be able to focus on real work. In some sense, I feel that I am fighting a really meaningless battle, even the original proposers have not said something on this patch - Why do I still care about the resolution so much? The all things make me feel I can't really make a fair decision on this patch. I regret that I was involved in the original implementation. I would definitely better if I were just a reviewer)

I sent the patch instead of reverting 3 or 4 intertwined dependent patches because I think reverting will just make the situation more confusing. I have experienced the review processes of all relevant patches and have read all the context,
so I very responsibly state again that "a simple revert of 3 or 4 dependent patches will just make the situation more confusing".

I will very appreciate feedback from other people, not my colleague, especially LLD contributors such as @psmith @grimar @jhenderson, whether we really need a flag in this patch or can we simply defer it to another thread.

With all respect: that we are colleagues is irrelevant here. This is merely a part of how the llvm project works. If we can't reach consensus, and we haven't, with multiple requests from contributors to revert the process is to revert. Please follow that process.

Thanks.

[MaskRay]

Change thunk
Reword description

[jhenderson]

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, @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.

[avl]

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.

It looks like there still exist a small difference with using -2 and 1 for thombstone value for debug_ranges. When base address selection entry is specified using -2 as thomstone value

{-1, -2} <<< base address selection entry
{0, length} <<< address range

the -2 value would case overflow and there would be situation when LowPC is greater than HighPC. This would not happen for 1 value. That overflow situation could affect customers who did not have it before. From that point of view using 1(BFD-identical) as thombstone value looks safer.

{-1, 1} <<< base address selection entry
{0, length} <<< address range

[jhenderson]

In D84825#2199023, @avl wrote:

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.

It looks like there still exist a small difference with using -2 and 1 for thombstone value for debug_ranges. When base address selection entry is specified using -2 as thomstone value

{-1, -2} <<< base address selection entry
{0, length} <<< address range

the -2 value would case overflow and there would be situation when LowPC is greater than HighPC. This would not happen for 1 value. That overflow situation could affect customers who did not have it before. From that point of view using 1(BFD-identical) as thombstone value looks safer.

{-1, 1} <<< base address selection entry
{0, length} <<< address range

That is a fair argument for the "1" approach for .debug_ranges (although I doubt in practice it makes any difference). It doesn't really help .debug_loc though, since the behaviour there is currently broken in BFD land.

[avl]

That is a fair argument for the "1" approach for .debug_ranges (although I doubt in practice it makes any difference). It doesn't really help .debug_loc though, since the behaviour there is currently broken in BFD land.

Agreed. So, using 1 for .debug_ranges and .debug_loc looks like safest solution.

[grimar]

In D84825#2198941, @jhenderson wrote:

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.

+1.

[MaskRay]

use 0 and 1

[echristo]

This looks good to me. Thanks for seeing this through Ray!

-eric

[dblaikie]

Looks good - thanks!

[MaskRay]

@hans Pushed to trunk. One test does not exist in release/11.x so this needs to resolve a conflict. I can push to release/11.x if you authorize it:)

[echristo]

*thumbs up on ok for release*

[hans]

In D84825#2201313, @MaskRay wrote:

@hans Pushed to trunk. One test does not exist in release/11.x so this needs to resolve a conflict. I can push to release/11.x if you authorize it:)

Sounds great, go ahead. Thanks!

lld/ELF/InputSection.cpp

if (tombstone ||
       // If the referenced symbol is discarded (made Undefined), or the
       // section defining the referenced symbol is garbage collected,
       // sym.getOutputSection() is nullptr. `ds->section->repl != ds->section`
       // catches the ICF folded case. However, resolving a relocation in
       // .debug_line to -1 would stop debugger users from setting breakpoints on
       // the folded-in function, so exclude .debug_line.
       //
       // For pre-DWARF-v5 .debug_loc and .debug_ranges, -1 is a reserved value
-      // (base address selection entry), so -2 is used.
+      // (base address selection entry), use 1 (which is used by GNU ld for
+      // .debug_ranges).
+      //
+      // TODO To reduce disruption, we use 0 instead of -1 as the tombstone
+      // value. Enable -1 in a future release.
       auto *ds = dyn_cast<Defined>(&sym);
       if (!sym.getOutputSection() ||
           (ds && ds->section->repl != ds->section && !isDebugLine)) {
         // If -z dead-reloc-in-nonalloc= is specified, respect it.
-        const uint64_t value =
-            tombstone ? SignExtend64<bits>(*tombstone)
-                      : (isDebugLocOrRanges ? UINT64_MAX - 1 : UINT64_MAX);
+        const uint64_t value = tombstone ? SignExtend64<bits>(*tombstone)
+                                         : (isDebugLocOrRanges ? 1 : 0);
         target->relocateNoSym(bufLoc, type, value);
         continue;
       }
     }
     target->relocateNoSym(bufLoc, type, SignExtend64<bits>(sym.getVA(addend)));
   }
 }
 

lld/test/ELF/dead-reloc-in-nonalloc.s

 # REQUIRES: x86
 ## Test that -z dead-reloc-in-nonalloc= can customize the tombstone value we
 ## use for an absolute relocation referencing a discarded symbol.
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
 # RUN: ld.lld --icf=all -z dead-reloc-in-nonalloc=.debug_info=0xaaaaaaaa \
 # RUN:   -z dead-reloc-in-nonalloc=.not_debug=0xbbbbbbbb %t.o -o %t
 # RUN: llvm-objdump -s %t | FileCheck %s --check-prefixes=COMMON,AA
 ## 0xaaaaaaaa == 2863311530
 # RUN: ld.lld --icf=all -z dead-reloc-in-nonalloc=.debug_info=2863311530 \
 # RUN:   -z dead-reloc-in-nonalloc=.not_debug=0xbbbbbbbb %t.o -o - | cmp %t -
 
 # COMMON:      Contents of section .debug_addr:
-# COMMON-NEXT:  0000 [[ADDR:[0-9a-f]+]] 00000000 ffffffff ffffffff
+# COMMON-NEXT:  0000 [[ADDR:[0-9a-f]+]] 00000000 00000000 00000000
 
 # AA:          Contents of section .debug_info:
 # AA-NEXT:      0000 [[ADDR]] 00000000 aaaaaaaa 00000000
 # AA:          Contents of section .not_debug:
 # AA-NEXT:      0000 bbbbbbbb
 
 ## Specifying zero can get a behavior similar to GNU ld.
 # RUN: ld.lld --icf=all -z dead-reloc-in-nonalloc=.debug_info=0 %t.o -o %tzero

lld/test/ELF/debug-dead-reloc-32.s

 # REQUIRES: x86
 ## Test we resolve symbolic relocations in .debug_* sections to a tombstone
 ## value if the referenced symbol is discarded (--gc-sections, non-prevailing
 ## section group, SHF_EXCLUDE, /DISCARD/, etc).
 
 # RUN: llvm-mc -filetype=obj -triple=i386 %s -o %t.o
 # RUN: ld.lld %t.o -o %t
 # RUN: llvm-objdump -s %t | FileCheck %s
 
 # CHECK:      Contents of section .debug_loc:
-# CHECK-NEXT:  0000 feffffff
+# CHECK-NEXT:  0000 01000000
 # CHECK-NEXT: Contents of section .debug_ranges:
-# CHECK-NEXT:  0000 feffffff
+# CHECK-NEXT:  0000 01000000
 # CHECK-NEXT: Contents of section .debug_addr:
-# CHECK-NEXT:  0000 ffffffff
+# CHECK-NEXT:  0000 00000000
 
 .section .text.1,"axe"
   .byte 0
 
 ## Resolved to UINT32_C(-2), with the addend ignored.
 ## UINT32_C(-1) is a reserved value (base address selection entry) which can't be used.
 .section .debug_loc
   .long .text.1+8
 .section .debug_ranges
   .long .text.1+16
 
-## Resolved to UINT32_C(-1), with the addend ignored.
+## Resolved to UINT32_C(0), with the addend ignored.
 .section .debug_addr
   .long .text.1+8

lld/test/ELF/debug-dead-reloc-icf.s

 # REQUIRES: x86
 ## Test we resolve symbolic relocations in .debug_* sections to a tombstone
 ## value if the referenced section symbol is folded into another section by ICF.
 ## Otherwise, we would leave entries in multiple CUs claiming ownership of the
 ## same range of code, which can confuse consumers.
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
 # RUN: ld.lld --icf=all %t.o -o %t
 # RUN: llvm-objdump -s %t | FileCheck %s
 
 # CHECK:      Contents of section .debug_info:
-# CHECK-NEXT:  0000 {{[0-9a-f]+}}000 00000000 ffffffff ffffffff
+# CHECK-NEXT:  0000 {{[0-9a-f]+}}000 00000000 00000000 00000000
 # CHECK:      Contents of section .debug_line:
 # CHECK-NEXT:  0000 [[ADDR:[0-9a-f]+]] 00000000
 # CHECK-SAME:                                   [[ADDR]] 00000000
 
 .globl _start
 _start:
   ret
 

lld/test/ELF/debug-dead-reloc-tls-arm.s

 # REQUIRES: arm
 ## Test we resolve relocations referencing TLS symbols in .debug_* sections to
 ## a tombstone value if the referenced TLS symbol is discarded.
 
 # RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
 # RUN: ld.lld --gc-sections %t.o -o %t
 # RUN: llvm-objdump -s %t | FileCheck %s
 
 # CHECK:      Contents of section .debug_info:
-# CHECK-NEXT:  0000 ffffffff
+# CHECK-NEXT:  0000 00000000
 
 .globl _start
 _start:
   bx lr
 
 .section .tbss,"awT",%nobits
 .globl tls
   .long 0
 
 .section .debug_info
 ## R_ARM_TLS_LDO32
   .long tls(tlsldo)

lld/test/ELF/debug-dead-reloc-tls.s

 # REQUIRES: x86
 ## Test we resolve relocations referencing TLS symbols in .debug_* sections to
 ## a tombstone value if the referenced TLS symbol is discarded.
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
 # RUN: ld.lld --gc-sections %t.o -o %t
 # RUN: llvm-objdump -s %t | FileCheck %s
 
 # CHECK:      Contents of section .debug_info:
-# CHECK-NEXT:  0000 ffffffff ffffffff ffffffff ffffffff
-# CHECK-NEXT:  0010 ffffffff ffffffff
+# CHECK-NEXT:  0000 00000000 00000000 00000000 00000000
+# CHECK-NEXT:  0010 00000000 ffffffff
 
 .globl _start
 _start:
   ret
 
 .section .tbss,"awT",@nobits
 .globl global
 local:

lld/test/ELF/debug-dead-reloc.s

 # REQUIRES: x86
 ## Test we resolve symbolic relocations in .debug_* sections to a tombstone
 ## value if the referenced symbol is discarded (--gc-sections, non-prevailing
 ## section group, SHF_EXCLUDE, /DISCARD/, etc).
 
 # RUN: echo '.globl _start; _start: call group' | llvm-mc -filetype=obj -triple=x86_64 - -o %t.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t1.o
 # RUN: ld.lld --gc-sections %t.o %t1.o %t1.o -o %t
 # RUN: llvm-objdump -s %t | FileCheck %s
 
 # CHECK:      Contents of section .debug_loc:
-# CHECK-NEXT:  0000 feffffff ffffffff feffffff ffffffff
+# CHECK-NEXT:  0000 01000000 00000000 01000000 00000000
 # CHECK-NEXT: Contents of section .debug_ranges:
-# CHECK-NEXT:  0000 feffffff ffffffff feffffff ffffffff
+# CHECK-NEXT:  0000 01000000 00000000 01000000 00000000
 # CHECK-NEXT: Contents of section .debug_addr:
 # CHECK-NEXT:  0000 {{.*}}000 00000000 {{.*}}000 00000000
-# CHECK-NEXT:  0010 ffffffff  ffffffff {{.*}}000 00000000
+# CHECK-NEXT:  0010 00000000  00000000 {{.*}}000 00000000
 # CHECK-NEXT: Contents of section .debug_foo:
-# CHECK-NEXT:  0000 ffffffff ffffffff 08000000 00000000
-# CHECK-NEXT:  0010 ffffffff ffffffff 08000000 00000000
+# CHECK-NEXT:  0000 00000000 00000000 08000000 00000000
+# CHECK-NEXT:  0010 00000000 00000000 08000000 00000000
 
 ## -z dead-reloc-in-nonalloc= can override the tombstone value.
 # RUN: ld.lld --gc-sections -z dead-reloc-in-nonalloc=.debug_loc=42 %t.o %t1.o %t1.o -o %t42
 # RUN: llvm-objdump -s %t42 | FileCheck %s --check-prefix=OVERRIDE
 
 # OVERRIDE:      Contents of section .debug_loc:
 # OVERRIDE-NEXT:  0000 2a000000 00000000 2a000000 00000000
 
 .section .text.1,"ax"
   .byte 0
 .section .text.2,"axe"
   .byte 0
 .section .text.3,"axG",@progbits,group,comdat
 .globl group
 group:
   .byte 0
 
-## Resolved to UINT64_C(-2), with the addend ignored.
+## Resolved to UINT64_C(1), with the addend ignored.
 ## UINT64_C(-1) is a reserved value (base address selection entry) which can't be used.
 .section .debug_loc
   .quad .text.1+8
 .section .debug_ranges
   .quad .text.2+16
 
 .section .debug_addr
 ## .text.3 is a local symbol. The symbol defined in a non-prevailing group is
-## discarded. Resolved to UINT64_C(-1).
+## discarded. Resolved to UINT64_C(0).
   .quad .text.3+24
 ## group is a non-local symbol. The relocation from the second %t1.o gets
 ## resolved to the prevailing copy.
   .quad group+32
 
 .section .debug_foo
   .quad .text.1+8
 
 ## We only deal with DW_FORM_addr. Don't special case short-range absolute
 ## relocations. Treat them like regular absolute relocations referencing
 ## discarded symbols, which are resolved to the addend.
   .long .text.1+8
   .long 0