Page MenuHomePhabricator

Emit 'no line' information for interesting 'orphan' instructions
ClosedPublic

Authored by probinson on Sep 1 2016, 6:33 PM.

Details

Summary

(Based on D16569 by friss; that revision was 'abandoned' so I needed to
create a new one. The following description is derived from his.)

There will always be instructions with no line information attached.
One example is the LocalValue constants generated by FastISel, but
it's by far not the only one. Compiler generated code will have no
line info and might get inlined. SelectionDAG is full of nodes that
won't have any line information (because they could be shared).

Multiple attempts have been made over time to fix the FastISel issue,
but none have felt satisfactory enough to be committed. As the problem
is much more general than just FastISel, this patch addresses it
at line table emission time rather than instruction selection time.

It also doesn't try to give accurate line information for these
instructions, but rather marks them with a 'no line' indicator so that
the debugger can safely ignore them.

The implemented heuristic will add 'no line' information to every insn
without explicit line information and either:

  • at the beginning of a basic-block
  • first instruction after a label

The first condition handles the FastISel case, while the second
enforces that any instruction referenced by anything (including debug
info) has line information.

The patch handles both DWARF and Codeview, using line number 0 for
DWARF and the reserved 'NeverStepIntoLineNumber' value for Codeview.
(I know almost nothing about Codeview so that might not be best...)

The initial measurements on DWARF line table size show an impact
between 5% and 15% (the latter is for a control-flow heavy code
eg. ASANified).

There was already an option to use DWARF 'line 0' information for all
the instructions with no debug locations. The commit history for this
code show that it was disabled because it caused issues with GDB and
with the line table size. Note that this code was unconditionally
emitting the line 0 information and not using a heuristic to find
interesting instructions like this one.

This patch has been tested on GDB-7.8 and current LLDB (pretty
sure I got the LLDB part right, but it's hard to tell). GDB showed no
differences, and LLDB had one XPASS (but I am not sure that isn't
just a fluke, as it was 'test_process_interrupt_dwarf' and I've had
flakiness problems with GDB thread-related tests in the past).
I don't know how to test the Codeview side.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson updated this revision to Diff 70125.Sep 1 2016, 6:33 PM
probinson retitled this revision from to Emit 'no line' information for interesting 'orphan' instructions.
probinson updated this object.
probinson added reviewers: echristo, aprantl, rnk.
probinson added subscribers: llvm-commits, friss.
rnk edited edge metadata.Sep 16 2016, 11:08 AM

I don't know if you have the time or interest in doing this, but here's how I'd test the codeview side.

First, compile something like a main function with ~5 volatile stores to a .s file with clang -g -gcodeview foo.c -o foo.s. Manually insert nops and .cv_loc directives in between the stores. Assemble it and link it into a complete program with clang. Run windbg -Q foo.exe, break on foo!main, and step by line from there and see what happens.

Once we do that, then we can know what NeverStepIntoLineNumber really does. Right now, I have no idea what it does, and without some information about that, I'd rather not record anything for these kinds of instructions so that we can have more compact line tables.

Thanks for the codeview recipe! I'll try that when I get a chance.

davide added a subscriber: davide.Oct 3 2016, 2:40 AM
In D24180#545101, @rnk wrote:

I don't know if you have the time or interest in doing this, but here's how I'd test the codeview side.

Even with spending most of a day on it, I completely fail at getting windbg to find 'main' and set a breakpoint. Even for a test program built with Visual Studio, I can't figure out how to persuade windbg to find the symbols.

Once we do that, then we can know what NeverStepIntoLineNumber really does. Right now, I have no idea what it does, and without some information about that, I'd rather not record anything for these kinds of instructions so that we can have more compact line tables.

I guess that means I should undo the CV parts of this patch.

We will need to figure out *something* for CV, because it is already the case that some places are explicitly setting a DebugLoc with line 0, and actually emitting line 0 into CV seems like the wrong result.

probinson updated this revision to Diff 75364.Oct 20 2016, 4:28 PM
probinson edited edge metadata.

Updated patch to take care of four things.

  1. Internal testing came up with situations where the patch would emit multiple line-0 records in a row. We now avoid that.
  1. We also found cases along these lines:
.loc 1 9 0
instr1
instr2
.loc 1 0 0
instr3
.loc 1 9 0
instr4

The naive statement-marking algorithm would put is_stmt 1 on both instr1 and instr4, which is not good for debugging. The updated patch detects this situation and puts is_stmt 0 on instr4.

  1. We aren't sure what to emit for CodeView, so as advised by @rnk this patch now does nothing for CodeView.
  1. The updated patch exposed an inadvertent flaw in test/CodeGen/X86/stack-protector.ll where a function marked as having debug info actually had no source location attached to any instructions. This led to emitting .loc 1 0 0 prologue_end which is obviously flawed. Attaching an actual source location to one of the instructions emits a much more sensible line table.
rnk added a comment.Oct 24 2016, 4:54 PM

I'm fine leaving CV alone for now until we learn more.

lib/CodeGen/AsmPrinter/DebugHandlerBase.h
46 ↗(On Diff #75364)

If you initialize this here to nullptr, you don't have to change the constructor.

Haven't fully followed the change, but did some tests at least - about a 7-7.5% regression (compressed or uncompressed) on a large program at Google. The line table's already larger (in absolute, and relative to the .text size (which is about about 11% smaller for LLVM)) than GCC's, but not by much (also somewhat not surprising that smaller .text might have larger line table - code might've been shuffled around more, have finer grained line table entries, etc).

Seems acceptable to me - just need to understand the code better when I or someone else gets a chance.

I was wondering if maybe I should restructure the DwarfDebug.cpp bit. Currently it's like

beginInstruction() {
  if (!MI->isDebugValue()) {
    if (DL != PrevInstLoc) {
      if (DL) {
        // case for a new, explicit location
      } else if (somewhat complicated condition) {
        // case for emitting line-0
      }
    } else if (DL) {
      if (DL.getLine() != LastAsmLine) {
        // case for restoring a previous line after emitting line-0
      }
    }
  }
} // end of function

and this could easily be redone as a sequence of much-less-indented cases:

beginInstruction() {
  if (MI->isDebugValue())
    return;
  if (DL == PrevInstLoc) {
    if (DL && DL.getLine() != LastAsmLine) {
      // case for restoring a previous line after emitting line-0
    }
    return;
  }
  // DL != PrevInstLoc
  if (DL) {
    // case for a new, explicit location
    return;
  }
  if (somewhat complicated condition) {
    // case for emitting line-0
  }
}

Would that help? The diff would be harder to read but the result might make more sense.

probinson updated this revision to Diff 78964.Nov 22 2016, 3:33 PM

Refactor patch after r287686 (D26982). Also took this opportunity to try to clarify the commentary.
Hopefully it's all rather easier to follow now.

dblaikie accepted this revision.Nov 23 2016, 2:38 PM
dblaikie added a reviewer: dblaikie.

Looks good (you could reduce some indentation in nthe newly introduced blocks if you like by using "if (!cond) return; work; return; " rather than "if (cond) { work; }" but leave it up to you if it feels better or not.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1041–1049 ↗(On Diff #78964)

I would've expected this conditional to be:

if (UnknownLocations || (...))

(essentially UnknownLocations triggers this in all cases, otherwise we trigger it in the subset of cases you're trying to implement (at the beginning of BB's, etc))

I'm assuming recordSourceLine does the appropriate thing to avoid producing redundant entries (eg: calling it with 0 twice is a no-op) - so the reordering of the condition you've done here is a minor optimization (to avoid calling recordSourceLine(0..) when it was already called last anyway), not a functional change?

1046 ↗(On Diff #78964)

Yeah - we have column info off by default (& it does make the line table significantly larger) if I recall correctly. But would be nice to have this improvement to this change at some point.

This revision is now accepted and ready to land.Nov 23 2016, 2:38 PM
probinson marked an inline comment as done.Nov 29 2016, 2:50 PM

Looks good (you could reduce some indentation in nthe newly introduced blocks if you like by using "if (!cond) return; work; return; " rather than "if (cond) { work; }" but leave it up to you if it feels better or not.

I did a little bit of that, it does help break out the cases better. Thanks!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1041–1049 ↗(On Diff #78964)

Actually no, recordSourceLine does not suppress duplicates. And because I don't set PrevInstLoc = DL; here anymore, I have to use LastAsmLine to determine whether I've already emitted a line-0 record.
But I think it does make sense to pull out

if (LastAsmLine == 0)
  return;

separately, it makes the commentary flow better and makes UnknownLocations etc condition easier to understand.

1046 ↗(On Diff #78964)

Can do.

This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.