This is an archive of the discontinued LLVM Phabricator instance.

Differential D74781

Wasm-ld emits invalid .debug_ranges entries for non-live symbols
ClosedPublic

Authored by paolosev on Feb 18 2020, 11:14 AM.

Details

Reviewers
sbc100
kripken
dschuff
Commits
rGaff75e1a1fac: [lld][Wasm] Wasm-ld emits invalid .debug_ranges entries for non-live symbols
Summary

When the debug info contains a relocation against a dead symbol, wasm-ld emits a range-list terminator entry (an entry with Start==0 and End==0).
This is not correct, because it incorrectly terminates the range list of that compilation unit, hiding the valid range entries that come after that invalid entry.

The behavior is different when compiling to native, in that case we get entries with Start==0 and End > 0:

.debug_ranges section
00004280 0000000000231030 000000000023105a
00004280 0000000000000000 0000000000000024
00004280 0000000000000000 000000000000007a
00004280 0000000000231060 00000000002310a7
...

while with Wasm we see range entries (0, 0) that spuriously terminate the sequence:

.debug_ranges contents:
00000000 000002ef 0000032d
00000000 00000357 00000360
00000000 <End of list>
00000028 000004ba 000004ff
00000028 00000576 000005c1
00000028 <End of list>
...

This patch fixes this problem by emitting the WasmRelocation Addend as End value for a non-live symbol.

Diff Detail

Event Timeline

paolosev created this revision.Feb 18 2020, 11:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2020, 11:14 AM
Herald added subscribers: llvm-commits, sunfish, aheejin, aprantl. · View Herald Transcript
sbc100 added a comment.Mar 17 2020, 4:56 PM

Would a more generic fix be to have calcNewValue include the addend on line 173 since this already mentions that it is there mostly for the purposes of debug info.

paolosev updated this revision to Diff 251876.Mar 21 2020, 10:59 PM

Thank you Sam, fixing this in calcNewValue also works and it's cleaner.

Harbormaster completed remote builds in B50020: Diff 251876.Mar 21 2020, 11:26 PM
sbc100 accepted this revision.Mar 22 2020, 9:12 AM

It might be nice to have a test for this? Did this change not effect any of the existing tests?

This revision is now accepted and ready to land.Mar 22 2020, 9:12 AM
sbc100 added a comment.Mar 22 2020, 9:12 AM

Looks good to me % testing

dblaikie added a subscriber: dblaikie.Mar 22 2020, 12:33 PM

You probably want to check on the behavior of debug_loc too, it also uses start/end pairs of zeros to terminate the list. I don't think there's any debug info LLVM produces currently that would trip over this (it'd need something like a global variable being optimized into local storage in a function that was optimized away to trip over this, for instance).

paolosev updated this revision to Diff 252209.Mar 23 2020, 8:32 PM

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Herald added a subscriber: jgravelle-google. · View Herald TranscriptMar 23 2020, 8:32 PM
Harbormaster completed remote builds in B50205: Diff 252209.Mar 23 2020, 9:13 PM
dblaikie added a comment.Mar 24 2020, 12:13 PM
In D74781#1938336, @paolosev wrote:

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Maybe it's not applicable to wasm, I don't know much about it - but if it optimizes variables into registers (or out of them), then it shuold be producible.

Here's a simple test case I use for debug_loc in C++ on a silicon target:

void f1();
int main() {
  int i = 3;
  f1();
  i = 7;
  f1();
}

Compiled with optimizations enabled, the storage for 'i' will be eliminated and a DWARF location list will be used to describe 'i' as having the value 3 over the first call instruction, and the value 7 over the second call instruction.

sbc100 added inline comments.Mar 24 2020, 1:49 PM
llvm/test/DebugInfo/WebAssembly/dbg-ranges-nonlive-symbols.test
1 ↗(On Diff #252209)

I think this needs to be a wasm-ld test doesn't it?

I think it should live in lld/test/wasm. Also the test inputs should be .ll if possible to avoid depending on clang. Also, the first input can be inline within the test file itself (dbg-ranges-nonlive-symbols.ll).

Alternative we could modify an existing test.. wait there is already: lld/test/wasm/debug-removed-fn.ll. Is this change not already covered by that?

paolosev marked 2 inline comments as done.Mar 24 2020, 8:24 PM
In D74781#1939787, @dblaikie wrote:
In D74781#1938336, @paolosev wrote:

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Maybe it's not applicable to wasm, I don't know much about it - but if it optimizes variables into registers (or out of them), then it shuold be producible.

Here's a simple test case I use for debug_loc in C++ on a silicon target:

void f1();
int main() {
  int i = 3;
  f1();
  i = 7;
  f1();
}

Compiled with optimizations enabled, the storage for 'i' will be eliminated and a DWARF location list will be used to describe 'i' as having the value 3 over the first call instruction, and the value 7 over the second call instruction.

Yes, I managed to get a .debug_loc section also for Wasm with similar code, and debug_loc does not seem to present the same problems with GC'd functions.

 1 void f1(int& n) { n++; }
 2
 3 int f2(int& n) {
 4   int i = 42;
 5   f1(i);
 6   i = 21;
 7   f1(i);
 8   return i;
 9  }
10
11 extern "C" {
12 int _start() {
13   int i = 3;
14   f1(i);
15   i = 7;
16   f1(i);
17   return i;
18 }
19 }

compiled with: clang++ main.cc -o main.wasm --target=wasm32 -nostdlib -gfull -O1, produce a module with this DWARF data:

.debug_info contents:
[...]
0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 11.0.0 (https://github.com/llvm/llvm-project.git f6cf848a2ccc6f63ebce27a52857c291a7ec4eec)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("main.cc")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("C:\\dev\\llvm-project\\release_x64\\T")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_ranges      (0x00000000
                 [0x00000002, 0x00000011)
                 [0x00000000, 0x0000004d)
                 [0x00000012, 0x0000005f))

0x00000026:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000002)
                DW_AT_high_pc   (0x0000000000000011)
                DW_AT_frame_base        (DW_OP_WASM_location 0x1 +0, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_linkage_name      ("_Z2f1Ri")
                DW_AT_name      ("f1")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (1)
                DW_AT_external  (true)
[...]

0x0000004a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000004d)
                DW_AT_frame_base        (DW_OP_WASM_location 0x0 +1, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_linkage_name      ("_Z2f2Ri")
                DW_AT_name      ("f2")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (3)
                DW_AT_type      (0x000000cd "int")
                DW_AT_external  (true)
[...]

0x00000071:     DW_TAG_variable
                  DW_AT_location        (0x00000000:
                     [0x00000014, 0x0000001b): DW_OP_consts +42, DW_OP_stack_value
                     [0x00000026, 0x0000002d): DW_OP_consts +21, DW_OP_stack_value
                     [0x0000003f, 0x0000004d): DW_OP_WASM_location 0x0 +2, DW_OP_stack_value)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("C:\dev\llvm-project\release_x64\T\main.cc")
                  DW_AT_decl_line       (4)
                  DW_AT_type    (0x000000cd "int")
[...]

0x00000093:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000012)
                DW_AT_high_pc   (0x000000000000005f)
                DW_AT_frame_base        (DW_OP_WASM_location 0x0 +0, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_name      ("_start")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (12)
                DW_AT_type      (0x000000cd "int")
                DW_AT_external  (true)

0x000000ab:     DW_TAG_variable
                  DW_AT_location        (0x00000038:
                     [0x00000026, 0x0000002d): DW_OP_consts +3, DW_OP_stack_value
                     [0x00000038, 0x0000003f): DW_OP_consts +7, DW_OP_stack_value
                     [0x00000051, 0x0000005f): DW_OP_WASM_location 0x0 +1, DW_OP_stack_value)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("C:\dev\llvm-project\release_x64\T\main.cc")
                  DW_AT_decl_line       (13)
                  DW_AT_type    (0x000000cd "int")
[...]

.debug_loc contents:
0x00000000:
            [0x00000014, 0x0000001b): DW_OP_consts +42, DW_OP_stack_value
            [0x00000026, 0x0000002d): DW_OP_consts +21, DW_OP_stack_value
            [0x0000003f, 0x0000004d): DW_OP_WASM_location 0x0 +2, DW_OP_stack_value

0x00000038:
            [0x00000026, 0x0000002d): DW_OP_consts +3, DW_OP_stack_value
            [0x00000038, 0x0000003f): DW_OP_consts +7, DW_OP_stack_value
            [0x00000051, 0x0000005f): DW_OP_WASM_location 0x0 +1, DW_OP_stack_value
[...]

.debug_line contents:
[...]
Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000002      1      0      1   0             0  is_stmt
0x0000000000000003      1     20      1   0             0  is_stmt prologue_end
0x0000000000000010      1     24      1   0             0
0x0000000000000011      1     24      1   0             0  end_sequence
0x0000000000000000      3      0      1   0             0  is_stmt
0x0000000000000014      4      7      1   0             0  is_stmt prologue_end
0x000000000000001b      5      3      1   0             0  is_stmt
0x0000000000000026      6      5      1   0             0  is_stmt
0x000000000000002d      7      3      1   0             0  is_stmt
0x0000000000000038      8     10      1   0             0  is_stmt
0x000000000000003f      8      3      1   0             0
0x000000000000004d      8      3      1   0             0  end_sequence
0x0000000000000012     12      0      1   0             0  is_stmt
0x0000000000000026     13      7      1   0             0  is_stmt prologue_end
0x000000000000002d     14      3      1   0             0  is_stmt
0x0000000000000038     15      5      1   0             0  is_stmt
0x000000000000003f     16      3      1   0             0  is_stmt
0x000000000000004a     17     10      1   0             0  is_stmt
0x0000000000000051     17      3      1   0             0
0x000000000000005f     17      3      1   0             0  end_sequence
[...]

.debug_ranges contents:
00000000 00000002 00000011
00000000 00000000 0000004d
00000000 00000012 0000005f
00000000 <End of list>

Function f2 is not used and removed by the linker. Now .debug_ranges is correct: there is a line
00000000 00000000 0000004d
which at least it's not interpreted as a terminator.

However, in the generated DWARF there is also useless/invalid information for f2 also in all the sections, with invalid code offsets.
I suspect that the presence of this data for removed functions could still break debuggers, which don't expect overlapping ranges.
It is also an useless waste of space. Would it be difficult to avoid generating DWARF data at all for GC'd functions?

llvm/test/DebugInfo/WebAssembly/dbg-ranges-nonlive-symbols.test
1 ↗(On Diff #252209)

Rght, I missed that test. And the change is rather simple.

paolosev updated this revision to Diff 252477.Mar 24 2020, 8:24 PM
paolosev marked an inline comment as done.
Harbormaster completed remote builds in B50350: Diff 252477.Mar 24 2020, 8:47 PM
sbc100 accepted this revision.Mar 25 2020, 6:35 PM
This comment was removed by sbc100.
dblaikie added a comment.Mar 26 2020, 12:39 PM
In D74781#1940564, @paolosev wrote:
In D74781#1939787, @dblaikie wrote:
In D74781#1938336, @paolosev wrote:

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Maybe it's not applicable to wasm, I don't know much about it - but if it optimizes variables into registers (or out of them), then it shuold be producible.

Here's a simple test case I use for debug_loc in C++ on a silicon target:

void f1();
int main() {
  int i = 3;
  f1();
  i = 7;
  f1();
}

Compiled with optimizations enabled, the storage for 'i' will be eliminated and a DWARF location list will be used to describe 'i' as having the value 3 over the first call instruction, and the value 7 over the second call instruction.

Yes, I managed to get a .debug_loc section also for Wasm with similar code, and debug_loc does not seem to present the same problems with GC'd functions.

Even without your patch? If without your patch you were getting range list entries of zero/zero, I'd expect you to get debug_loc entries with zero/zero too. Any ideas why it doesn't? They should be a similar pair of relocations (though it looks like your fix should work for debug_loc too, which is good).

 1 void f1(int& n) { n++; }
 2
 3 int f2(int& n) {
 4   int i = 42;
 5   f1(i);
 6   i = 21;
 7   f1(i);
 8   return i;
 9  }
10
11 extern "C" {
12 int _start() {
13   int i = 3;
14   f1(i);
15   i = 7;
16   f1(i);
17   return i;
18 }
19 }

compiled with: clang++ main.cc -o main.wasm --target=wasm32 -nostdlib -gfull -O1, produce a module with this DWARF data:

.debug_info contents:
[...]
0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 11.0.0 (https://github.com/llvm/llvm-project.git f6cf848a2ccc6f63ebce27a52857c291a7ec4eec)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("main.cc")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("C:\\dev\\llvm-project\\release_x64\\T")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_ranges      (0x00000000
                 [0x00000002, 0x00000011)
                 [0x00000000, 0x0000004d)
                 [0x00000012, 0x0000005f))

0x00000026:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000002)
                DW_AT_high_pc   (0x0000000000000011)
                DW_AT_frame_base        (DW_OP_WASM_location 0x1 +0, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_linkage_name      ("_Z2f1Ri")
                DW_AT_name      ("f1")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (1)
                DW_AT_external  (true)
[...]

0x0000004a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000004d)
                DW_AT_frame_base        (DW_OP_WASM_location 0x0 +1, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_linkage_name      ("_Z2f2Ri")
                DW_AT_name      ("f2")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (3)
                DW_AT_type      (0x000000cd "int")
                DW_AT_external  (true)
[...]

0x00000071:     DW_TAG_variable
                  DW_AT_location        (0x00000000:
                     [0x00000014, 0x0000001b): DW_OP_consts +42, DW_OP_stack_value
                     [0x00000026, 0x0000002d): DW_OP_consts +21, DW_OP_stack_value
                     [0x0000003f, 0x0000004d): DW_OP_WASM_location 0x0 +2, DW_OP_stack_value)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("C:\dev\llvm-project\release_x64\T\main.cc")
                  DW_AT_decl_line       (4)
                  DW_AT_type    (0x000000cd "int")
[...]

0x00000093:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000012)
                DW_AT_high_pc   (0x000000000000005f)
                DW_AT_frame_base        (DW_OP_WASM_location 0x0 +0, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_name      ("_start")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (12)
                DW_AT_type      (0x000000cd "int")
                DW_AT_external  (true)

0x000000ab:     DW_TAG_variable
                  DW_AT_location        (0x00000038:
                     [0x00000026, 0x0000002d): DW_OP_consts +3, DW_OP_stack_value
                     [0x00000038, 0x0000003f): DW_OP_consts +7, DW_OP_stack_value
                     [0x00000051, 0x0000005f): DW_OP_WASM_location 0x0 +1, DW_OP_stack_value)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("C:\dev\llvm-project\release_x64\T\main.cc")
                  DW_AT_decl_line       (13)
                  DW_AT_type    (0x000000cd "int")
[...]

.debug_loc contents:
0x00000000:
            [0x00000014, 0x0000001b): DW_OP_consts +42, DW_OP_stack_value
            [0x00000026, 0x0000002d): DW_OP_consts +21, DW_OP_stack_value
            [0x0000003f, 0x0000004d): DW_OP_WASM_location 0x0 +2, DW_OP_stack_value

0x00000038:
            [0x00000026, 0x0000002d): DW_OP_consts +3, DW_OP_stack_value
            [0x00000038, 0x0000003f): DW_OP_consts +7, DW_OP_stack_value
            [0x00000051, 0x0000005f): DW_OP_WASM_location 0x0 +1, DW_OP_stack_value
[...]

.debug_line contents:
[...]
Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000002      1      0      1   0             0  is_stmt
0x0000000000000003      1     20      1   0             0  is_stmt prologue_end
0x0000000000000010      1     24      1   0             0
0x0000000000000011      1     24      1   0             0  end_sequence
0x0000000000000000      3      0      1   0             0  is_stmt
0x0000000000000014      4      7      1   0             0  is_stmt prologue_end
0x000000000000001b      5      3      1   0             0  is_stmt
0x0000000000000026      6      5      1   0             0  is_stmt
0x000000000000002d      7      3      1   0             0  is_stmt
0x0000000000000038      8     10      1   0             0  is_stmt
0x000000000000003f      8      3      1   0             0
0x000000000000004d      8      3      1   0             0  end_sequence
0x0000000000000012     12      0      1   0             0  is_stmt
0x0000000000000026     13      7      1   0             0  is_stmt prologue_end
0x000000000000002d     14      3      1   0             0  is_stmt
0x0000000000000038     15      5      1   0             0  is_stmt
0x000000000000003f     16      3      1   0             0  is_stmt
0x000000000000004a     17     10      1   0             0  is_stmt
0x0000000000000051     17      3      1   0             0
0x000000000000005f     17      3      1   0             0  end_sequence
[...]

.debug_ranges contents:
00000000 00000002 00000011
00000000 00000000 0000004d
00000000 00000012 0000005f
00000000 <End of list>

Function f2 is not used and removed by the linker. Now .debug_ranges is correct: there is a line
00000000 00000000 0000004d
which at least it's not interpreted as a terminator.

However, in the generated DWARF there is also useless/invalid information for f2 also in all the sections, with invalid code offsets.
I suspect that the presence of this data for removed functions could still break debuggers, which don't expect overlapping ranges.
It is also an useless waste of space. Would it be difficult to avoid generating DWARF data at all for GC'd functions?

Yes, it's difficult for "Classic" targets like ELF/COFF/etc - the linker isn't DWARF aware (except for a couple of special cases like this relocation application - and in fact binutils ld does a different workaround for debug_ranges (it resolves all of them to 1 for dropped code, so you get a 1/1 entry rather than a 0/0 entry or a 0/N entry (like gold/lld)) & doesn't have a fix for debug_loc, which isn't exactly a problem except for DWARF dumping tools) & it's unclear that DWARF-aware linking is going to be worthwhile - it means linking probably using a lot more memory and processing time to reconstruct a bunch of DWARF (you can't just rip out the DIEs - there are other offsets that would then be invalidated and have to be rewritten, etc).

There is work to add DWARF-aware linking based on Apple dsymutil implementation in LLVM, to lld for ELF DWARF linking - but yeah, unclear it'll be an absolute win in terms of performance - and in terms of correctness, debuggers have been having to live with this "ranges that start at zero are magic/represent dropped code" for a while now & if that was shown to be really unworkable from a correctness standpoint we'd probably want to consider some other magic value, rather than requiring full DWARF rewriting to get correctness.

sbc100 added a comment.Mar 26 2020, 1:17 PM

Regardless of any longer term plans for a new type of DWARF linking it seems that this change is strictly an improvement since (a) it matches the behavior of the existing ELF linker and (b) existing tools already handle this 0 ... N range.

Any objections to landing this now?

dblaikie added a comment.Mar 26 2020, 1:38 PM
In D74781#1944828, @sbc100 wrote:

Regardless of any longer term plans for a new type of DWARF linking it seems that this change is strictly an improvement since (a) it matches the behavior of the existing ELF linker and (b) existing tools already handle this 0 ... N range.

Any objections to landing this now?

My feedback isn't intended to be an objection (though I think it might be informative to understand if this bug doesn't appear in debug_loc without this patch... that surprises me, but low priority I suppose as it looks like the patch's fix would apply to debug_loc as well as it would to debug_ranges). I'll leave it to other wasm-y folks to approve, etc.

sbc100 added a comment.Mar 26 2020, 2:11 PM

Regarding the .debug_loc section. I did a quick experiment it looks like, even before this patch we get the expected output:

test.o:	file format WASM

.debug_loc contents:
0x00000000: 
            [0x00000005, 0x00000009): DW_OP_consts +21, DW_OP_stack_value

0x0000001d: 
            [0x0000000a, 0x0000000e): DW_OP_consts +7, DW_OP_stack_value

It looks like reason this works is because there is only one relocation per function here. i.e. there are only two relocations in the above object code. I appears the individual ranges do not themselves contain any relocations.

paolosev added a comment.Mar 26 2020, 2:18 PM

Yes, I managed to get a .debug_loc section also for Wasm with similar code, and debug_loc does not seem to present the same problems with GC'd functions.

Even without your patch? If without your patch you were getting range list entries of zero/zero, I'd expect you to get debug_loc entries with zero/zero too. Any ideas why it doesn't? They should be a similar pair of relocations (though it looks like your fix should work for debug_loc too, which is good).

Looking at the generated DWARF, this patch does not change the offsets for debug_loc. Like in the example above, there are loc entries for variables in the deleted function f2:

0x0000004a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000004d)
                DW_AT_frame_base        (DW_OP_WASM_location 0x0 +1, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_linkage_name      ("_Z2f2Ri")
                DW_AT_name      ("f2")
                DW_AT_decl_file ("C:\dev\llvm-project\release_x64\T\main.cc")
                DW_AT_decl_line (3)
                DW_AT_type      (0x000000cd "int")
                DW_AT_external  (true)
[...]
0x00000071:     DW_TAG_variable
                  DW_AT_location        (0x00000000:
                     [0x00000014, 0x0000001b): DW_OP_consts +42, DW_OP_stack_value
                     [0x00000026, 0x0000002d): DW_OP_consts +21, DW_OP_stack_value
                     [0x0000003f, 0x0000004d): DW_OP_WASM_location 0x0 +2, DW_OP_stack_value)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("C:\dev\llvm-project\release_x64\T\main.cc")
                  DW_AT_decl_line       (4)
                  DW_AT_type    (0x000000cd "int")

The entries seems to assume that the deleted function starts at offset 0, and ends at 0x4d, and this is true also without the patch.

dblaikie added a comment.Mar 26 2020, 2:22 PM
In D74781#1944970, @sbc100 wrote:

Regarding the .debug_loc section. I did a quick experiment it looks like, even before this patch we get the expected output:

test.o:	file format WASM

.debug_loc contents:
0x00000000: 
            [0x00000005, 0x00000009): DW_OP_consts +21, DW_OP_stack_value

0x0000001d: 
            [0x0000000a, 0x0000000e): DW_OP_consts +7, DW_OP_stack_value

It looks like reason this works is because there is only one relocation per function here. i.e. there are only two relocations in the above object code. I appears the individual ranges do not themselves contain any relocations.

Hmm, what does llvm-dwarfdump -debug-loc -v print for this? I did implement DWARF loclist base address specifier support which means sometimes it'll use a base address specifier (which would be only one address) & everything would be offsets relative to that - but that's not always used.

Hmm, maybe I'm mistaken... maybe we do /always/ use base address specifiers in debug_loc now... Yep, that's it, always using them.

Sorry for the distraction & thanks for talking it through with me!

paolosev added a comment.Mar 26 2020, 2:26 PM
In D74781#1944988, @dblaikie wrote:
In D74781#1944970, @sbc100 wrote:

Regarding the .debug_loc section. I did a quick experiment it looks like, even before this patch we get the expected output:

test.o:	file format WASM

.debug_loc contents:
0x00000000: 
            [0x00000005, 0x00000009): DW_OP_consts +21, DW_OP_stack_value

0x0000001d: 
            [0x0000000a, 0x0000000e): DW_OP_consts +7, DW_OP_stack_value

It looks like reason this works is because there is only one relocation per function here. i.e. there are only two relocations in the above object code. I appears the individual ranges do not themselves contain any relocations.

Hmm, what does llvm-dwarfdump -debug-loc -v print for this? I did implement DWARF loclist base address specifier support which means sometimes it'll use a base address specifier (which would be only one address) & everything would be offsets relative to that - but that's not always used.

Hmm, maybe I'm mistaken... maybe we do /always/ use base address specifiers in debug_loc now... Yep, that's it, always using them.

Sorry for the distraction & thanks for talking it through with me!

Thanks for your help!
Then I will push the patch.

Closed by commit rGaff75e1a1fac: [lld][Wasm] Wasm-ld emits invalid .debug_ranges entries for non-live symbols (authored by Paolo Severini <paolosev@microsoft.com>). · Explain WhyMar 26 2020, 2:42 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
test/
wasm/
7 lines
wasm/
6 lines

Diff 252983

lld/test/wasm/debug-removed-fn.ll

; RUN: llc -filetype=obj < %s -o %t.o ; RUN: llc -filetype=obj < %s -o %t.o
; RUN: wasm-ld %t.o --no-entry --export=foo -o %t.wasm ; RUN: wasm-ld %t.o --no-entry --export=foo -o %t.wasm
; RUN: llvm-dwarfdump -debug-line %t.wasm | FileCheck %s ; RUN: llvm-dwarfdump -debug-line -debug-ranges %t.wasm | FileCheck %s
; CHECK: Address ; CHECK: Address
; CHECK: 0x0000000000000005 ; CHECK: 0x0000000000000005
; CHECK: 0x0000000000000000 ; CHECK: 0x0000000000000000
; CHECK: .debug_ranges contents:
; CHECK: 00000000 {{[0-9]+}} {{[0-9]+}}
; CHECK: 00000000 {{[0-9]+}} {{[0-9]+}}
; CHECK: 00000000 <End of list>
; ModuleID = 't.bc' ; ModuleID = 't.bc'
target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128" target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown-wasm" target triple = "wasm32-unknown-unknown-wasm"
; Function Attrs: noinline nounwind optnone ; Function Attrs: noinline nounwind optnone
define hidden i32 @foo() #0 !dbg !7 { define hidden i32 @foo() #0 !dbg !7 {
entry: entry:
ret i32 42, !dbg !11 ret i32 42, !dbg !11
Show All 28 Lines

lld/wasm/InputFiles.cpp

Show First 20 Lines • Show All 162 Lines • ▼ Show 20 Lines
// Translate from the relocation's index into the final linked output value. // Translate from the relocation's index into the final linked output value.
uint32_t ObjFile::calcNewValue(const WasmRelocation &reloc) const { uint32_t ObjFile::calcNewValue(const WasmRelocation &reloc) const {
const Symbol* sym = nullptr; const Symbol* sym = nullptr;
if (reloc.Type != R_WASM_TYPE_INDEX_LEB) { if (reloc.Type != R_WASM_TYPE_INDEX_LEB) {
sym = symbols[reloc.Index]; sym = symbols[reloc.Index];
// We can end up with relocations against non-live symbols. For example // We can end up with relocations against non-live symbols. For example
// in debug sections. // in debug sections. We return reloc.Addend because always returning zero
// causes the generation of spurious range-list terminators in the
// .debug_ranges section.
if ((isa<FunctionSymbol>(sym) || isa<DataSymbol>(sym)) && !sym->isLive()) if ((isa<FunctionSymbol>(sym) || isa<DataSymbol>(sym)) && !sym->isLive())
return 0; return reloc.Addend;
} }
switch (reloc.Type) { switch (reloc.Type) {
case R_WASM_TABLE_INDEX_I32: case R_WASM_TABLE_INDEX_I32:
case R_WASM_TABLE_INDEX_SLEB: case R_WASM_TABLE_INDEX_SLEB:
case R_WASM_TABLE_INDEX_REL_SLEB: { case R_WASM_TABLE_INDEX_REL_SLEB: {
if (!getFunctionSymbol(reloc.Index)->hasTableIndex()) if (!getFunctionSymbol(reloc.Index)->hasTableIndex())
return 0; return 0;
▲ Show 20 Lines • Show All 383 Lines • Show Last 20 Lines