Page MenuHomePhabricator

X86, AArch64, ARM: Do not attach debug location to spill/reload instructions
ClosedPublic

Authored by MatzeB on Sep 14 2018, 3:02 PM.

Details

Summary

Spill/reload instructions are artificially generated by the compiler and
have no relation to the original source code. So the best thing to do is
not attach any debug location to them (instead of just taking the next
debug location we find on following instructions).

(I stumbled upon this when working on the fast regalloc. At least to me it
felt odd that we do set a debug location on spill/reload instructions...)

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Sep 14 2018, 3:02 PM
MatzeB edited the summary of this revision. (Show Details)Sep 14 2018, 3:04 PM
MatzeB added reviewers: aprantl, vsk.
vsk added a comment.Sep 14 2018, 3:31 PM

As Matthias points out, a spill/reload can't unambiguously be associated with a specific instruction.

Note that passing an empty location means that the instruction is described by the previous .loc directive in the stream (if one is present). I think the pedantic thing to do would be to use the special "line 0" location. However, that might bloat the line table and possibly interfere with the way llvm currently identifies prologues.

I think this is the right approach. LGTM, although a regression test on the ARM side wouldn't hurt :).

This revision was not accepted when it landed; it landed in state Needs Review.Oct 1 2018, 11:58 AM
This revision was automatically updated to reflect the committed changes.
In D52125#1235612, @vsk wrote:

As Matthias points out, a spill/reload can't unambiguously be associated with a specific instruction.

Note that passing an empty location means that the instruction is described by the previous .loc directive in the stream (if one is present). I think the pedantic thing to do would be to use the special "line 0" location. However, that might bloat the line table and possibly interfere with the way llvm currently identifies prologues.

What would be the difference between a "line 0" location and an empty location? They both sound like "doesn't have a corresponding location in the source" to me...

I think this is the right approach. LGTM, although a regression test on the ARM side wouldn't hurt :).

added an aarch64 test.

vsk added a comment.Oct 1 2018, 12:26 PM
In D52125#1235612, @vsk wrote:

As Matthias points out, a spill/reload can't unambiguously be associated with a specific instruction.

Note that passing an empty location means that the instruction is described by the previous .loc directive in the stream (if one is present). I think the pedantic thing to do would be to use the special "line 0" location. However, that might bloat the line table and possibly interfere with the way llvm currently identifies prologues.

What would be the difference between a "line 0" location and an empty location? They both sound like "doesn't have a corresponding location in the source" to me...

A "line 0" location is used to describe compiler-generated instructions which nonetheless have meaningful lexical scopes and inlining data. E.g you could attach "line 0" to a conditional move instruction.

An empty location isn't a location at all, so it doesn't have a specific definition. LLVM generally handles instructions without debug locations by presuming that the "last seen" location is unchanged.

I think this is the right approach. LGTM, although a regression test on the ARM side wouldn't hurt :).

added an aarch64 test.

This change is breaking some symbolization tests on the Android bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/15630

Please fix or revert.

Feel free to revert for now, but are you sure it is actually this commit?

It's hard to say for sure what happenes with the earlier hwasan tests, but the later tests that failed are crashing in GlobalISel related code, which should all run before the code that I changed...

+CC dsanders

eugenis added a subscriber: eugenis.Oct 2 2018, 1:45 PM

It's a bug in HWASan where we symbolize the address of the previous instruction, as usual when going up the stack (call instruction is at function return address - 4), but fault address from the signal handler does not need this adjustment. The previous instruction happens to be a reload (with -O0).

I'll fix it.

I've noticed that your change increases the size of .debug_line with HWASan, -gline-tables-only and -O0 by ~50%. Perhaps this is the worst possible case, because HWASan inserts lots of short cold BBs, something like

if (unlikely(bad_address)) {x0 = addr; brk; unreachable; }

before each memory access, and at -O0 there is often a reload of addr in the cold branch.

This is what the line table looks like:
Contents of the .debug_line section:

CU: /code/llvm-project/compiler-rt/test/hwasan/TestCases/halt-on-error.cc:
File name Line number Starting address View
halt-on-error.cc 9 0
halt-on-error.cc 10 0xa8
halt-on-error.cc 11 0xb4
halt-on-error.cc 11 0xc0
halt-on-error.cc 0 0xec
halt-on-error.cc 11 0xf0
halt-on-error.cc 0 0xf4
halt-on-error.cc 11 0xfc
halt-on-error.cc 12 0x104
halt-on-error.cc 0 0x12c
halt-on-error.cc 12 0x130
halt-on-error.cc 0 0x134
halt-on-error.cc 12 0x138
halt-on-error.cc 12 0x13c
halt-on-error.cc 13 0x140
halt-on-error.cc 14 0x14c
halt-on-error.cc 0 0x174
halt-on-error.cc 14 0x178
halt-on-error.cc 0 0x17c
halt-on-error.cc 14 0x180
halt-on-error.cc 0 0x1bc
halt-on-error.cc 14 0x1c0
halt-on-error.cc 0 0x1c4
halt-on-error.cc 14 0x1c8
halt-on-error.cc 14 0x1d4
halt-on-error.cc 0 0x200
halt-on-error.cc 14 0x204
halt-on-error.cc 0 0x208
halt-on-error.cc 14 0x20c
halt-on-error.cc 14 0x210
halt-on-error.cc 0 0x248
halt-on-error.cc 14 0x24c
halt-on-error.cc 0 0x250
halt-on-error.cc 14 0x254
halt-on-error.cc 14 0x25c
halt-on-error.cc 14 0x268
halt-on-error.cc 0 0x294
halt-on-error.cc 14 0x298
halt-on-error.cc 0 0x29c
halt-on-error.cc 14 0x2a0
halt-on-error.cc 14 0x2a4
halt-on-error.cc 0 0x2dc
halt-on-error.cc 14 0x2e0
halt-on-error.cc 0 0x2e4
halt-on-error.cc 14 0x2e8
halt-on-error.cc 14 0x2f0
halt-on-error.cc 14 0x2f8
halt-on-error.cc 14 0x324

vsk added a comment.Oct 2 2018, 1:50 PM

It's a bug in HWASan where we symbolize the address of the previous instruction, as usual when going up the stack (call instruction is at function return address - 4), but fault address from the signal handler does not need this adjustment. The previous instruction happens to be a reload (with -O0).

I'll fix it.

I've noticed that your change increases the size of .debug_line with HWASan, -gline-tables-only and -O0 by ~50%. Perhaps this is the worst possible case, because HWASan inserts lots of short cold BBs, something like

if (unlikely(bad_address)) {x0 = addr; brk; unreachable; }

before each memory access, and at -O0 there is often a reload of addr in the cold branch.
[snip]

With this patch, the reload doesn't have a location. ISTM that the bug causing line table bloat is that the dwarf generator is emitting 0 locations when it shouldn't be.

In case someone wants to look at debug info issue:

test case
bin/clang++ -fsanitize=hwaddress --target=aarch64-linux-android -gline-tables-only -O0 1.ii -c

vsk added a comment.Oct 2 2018, 2:32 PM

In case someone wants to look at debug info issue:

test case
bin/clang++ -fsanitize=hwaddress --target=aarch64-linux-android -gline-tables-only -O0 1.ii -c

I can reproduce the issue. I think the unexpected locations come from DwargDebug::beginInstruction:

if (!DL) {
  // We have an unspecified location, which might want to be line 0.

Apparently there's a toggle which can switch off this behavior: -mllvm -use-unknown-locations=Disable.

... and trying again with that option set, the line table bloat is gone.

By default line 0 locations are enabled if the instruction is the first inst in a block, or is after a label. That seems reasonable, but in light of this issue it might be worth revisiting.

MatzeB added a comment.EditedOct 2 2018, 2:51 PM
In D52125#1253119, @vsk wrote:

In case someone wants to look at debug info issue:

test case
bin/clang++ -fsanitize=hwaddress --target=aarch64-linux-android -gline-tables-only -O0 1.ii -c

I can reproduce the issue. I think the unexpected locations come from DwargDebug::beginInstruction:

if (!DL) {
  // We have an unspecified location, which might want to be line 0.

Apparently there's a toggle which can switch off this behavior: -mllvm -use-unknown-locations=Disable.

... and trying again with that option set, the line table bloat is gone.

By default line 0 locations are enabled if the instruction is the first inst in a block, or is after a label. That seems reasonable, but in light of this issue it might be worth revisiting.

Interesting. I can understand that we don't want debug locs from the previous block to flow over. I guess we could take the next debug loc we can find (in the same basic block) in a case like this.
I'll look into this before attempting to re-commit this...

vsk added a comment.Oct 2 2018, 2:55 PM
In D52125#1253119, @vsk wrote:

In case someone wants to look at debug info issue:

test case
bin/clang++ -fsanitize=hwaddress --target=aarch64-linux-android -gline-tables-only -O0 1.ii -c

I can reproduce the issue. I think the unexpected locations come from DwargDebug::beginInstruction:

if (!DL) {
  // We have an unspecified location, which might want to be line 0.

Apparently there's a toggle which can switch off this behavior: -mllvm -use-unknown-locations=Disable.

... and trying again with that option set, the line table bloat is gone.

By default line 0 locations are enabled if the instruction is the first inst in a block, or is after a label. That seems reasonable, but in light of this issue it might be worth revisiting.

Interesting. I can understand that we don't want debug locs from the previous block to flow over. I guess we could take the next debug loc we can find (in the same basic block) in a case like this.
I'll look into this before attempting to re-commit this...

Thanks! That sounds reasonable to me. As an added benefit it doesn't seem likely to break very many tests.

I've fixed HWASan in r343638.

MatzeB added a comment.EditedOct 3 2018, 10:25 PM

Recommitted as rL343895 now, let's hope it passes the tests this time.