[PATCH] D147506: [DebugLine] save one debug line entry for empty prologue

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

After talking it through (thanks for your patience with this) I guess this is OK - the solution I ended up at below still involves fairly specific prologue handling anyway... though it doesn't explicitly have to detect if the prologue is empty and maybe reduces the explicit prologue handling in general too, but is maybe more bother than it's worth - if you want to have a go at that refactoring, it might be nice, but I won't hold up this patch on that work. Maybe I'll have a go at it myself if you don't get to it, after this patch is committed.

In D147506#4301056 <https://reviews.llvm.org/D147506#4301056>, @shchenz wrote:

>> That way if two recordSourceLine calls happen with no instruction between them, no emitDwarfLocDirective is called - but on the next insntruction that's emitted, the source line info would be emitted at that point.
>
>   recordSourceLine() {
>     this->lineInfo = {line, file};
>   }
>   beginInstruction() {
>     if (this->lineInfo) {
>       emitDwarfLocDirective(this->lineInfo)
>       this->lineInfo = std::nullopt;
>     }
>     ...
>   }
>
> Thanks! I know your point now. This should be able to handle duplicated lines for the empty prologue case. But seems it also has same issue for the case when generating `loc` for instructions. (Currently, when starting to handle instructions, `recordSourceLine` is always called by a new instruction emitting, so there are always instructions between two `recordSourceLine()` calls in `beginInstruction()`). For example:
>
>   Inst1;  // line1
>   Inst2;  // line2
>
> When handle `Inst1`, in the `DwarfDebug::beginInstruction()`, the line info for `Inst1` is cached to `this->lineInfo()` in `recordSourceLine()` and then `Inst1` is emitted to assembly file. This delay will cause issue, `loc` for `Inst1` is supposed to be in front of `Inst1`. Even at the very beginning of `beginInstruction()` for `Instr2`, we call `emitDwarfLocDirective()` for `Inst1`, it is still put the `.loc` after `Inst1`.

Ah, right, sorry. I'll try to clarify further.

Maybe a more accurate pseudocode would be this:

  beginInstruction() {
    if (this->InPrologue) {
      this->InPrologue = IsInPrologue(~= is frame setup and doesn't have a debug location);
      if (this->InPrologue) {
        if (!MetaInstruction) {
          this->PrologueLocationEmitted = true;
          apply the location we use for the prologue
        }
      }
    }
    ...
  }

So I guess it comes out to still being a special case for the prologue, unfortunately... and this ^ solution could rewrite the prologue handling code such that it doesn't need a specific walk ahead of time, the end of the prologue would be detected during the instruction walk. But requires an extra member variable/more mutable state. Not sure if it's substantially better.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2117-2130
       if (!MI.isMetaInstruction() && !MI.getFlag(MachineInstr::FrameSetup) &&
           MI.getDebugLoc()) {
         // Scan forward to try to find a non-zero line number. The prologue_end
         // marks the first breakpoint in the function after the frame setup, and
         // a compiler-generated line 0 location is not a meaningful breakpoint.
         // If none is found, return the first location after the frame setup.
         if (MI.getDebugLoc().getLine())
----------------
Perhaps this'd be written as:
```
if (MetaInstruction) {
  if (!FrameSetup &&DebugLoc) {
    ...
  }
  IsEmptyPrologue = false;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147506/new/

https://reviews.llvm.org/D147506