[PATCH] D83468: [Debuginfo] Fix for PR46653

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D83468#2226868 <https://reviews.llvm.org/D83468#2226868>, @Jac1494 wrote:

>>> Also possible it's not a bug at all & GlobalISel just ends up doing some optimization that does end up creating a necessary/correct line 0 at the start of the function where SelectionDAG/FastISel does not.
>
> @dblaikie  SelectionDAG/FastISel is also creating necessary/correct line zero. Also this line zero is user can control using "-use-unknown-locations=Disable" option as per below code.

I don't think that flag is for all line 0, I believe it's for instructions that have no line specified - and then that flag causes us to specifically force them to line 0 when such unspecified location instructions appear at the start of a block or a few other conditions as documented in the comments at its usage. So I don't think this flag will remove all line zeros/isn't intended to do that.

> But this options is not fully working for Global-isel because of incorrect line zeros.

What makes them incorrect? & what behavior of -use-unknown-locations is not "fully working" because of this? If you expect use-unknown-locations to remove all line 0 entries, I Think that's an incorrect expectation, not an incorrect behavior of LLVM.

> llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1964
>
>   if (!DL) {
>      // We have an unspecified location, which might want to be line 0.
>      // If we have already emitted a line-0 record, don't repeat it.
>      if (LastAsmLine == 0)
>        return;
>      // If user said Don't Do That, don't do that.
>      if (UnknownLocations == Disable)
>        return;
>      // See if we have a reason to emit a line-0 record now.
>      // Reasons to emit a line-0 record include:
>      // - User asked for it (UnknownLocations).
>      // - Instruction has a label, so it's referenced from somewhere else,
>      //   possibly debug information; we want it to have a source location.
>      // - Instruction is at the top of a block; we don't want to inherit the
>      //   location from the physically previous (maybe unrelated) block.
>      if (UnknownLocations == Enable || PrevLabel ||
>          (PrevInstBB && PrevInstBB != MI->getParent())) {
>        // Preserve the file and column numbers, if we can, to save space in
>        // the encoded line table.
>        // Do not update PrevInstLoc, it remembers the last non-0 line.
>        const MDNode *Scope = nullptr;
>        unsigned Column = 0;
>        if (PrevInstLoc) {
>          Scope = PrevInstLoc.getScope();
>          Column = PrevInstLoc.getCol();
>        }
>        recordSourceLine(/*Line=*/0, Column, Scope, /*Flags=*/0);
>      }
>      return;
>    }
>
> List of SelectionDAG/FastIsel base test cases are given below which creating meaningful/correct line zero.
>
>   CodeGen/X86/dbg-line-0-no-discriminator.ll
>   CodeGen/X86/stack-protector.ll
>   CodeGen/X86/unknown-location.ll
>   DebugInfo/AArch64/line-header.ll
>   DebugInfo/MIR/X86/debug-loc-0.mir
>   DebugInfo/X86/dbg-prolog-end.ll
>   DebugInfo/X86/dwarf-no-source-loc.ll
>   DebugInfo/X86/tail-merge.ll
>
>
>
>>> Best to look at the assembly/optimizations/etc and see why it's getting line zero and decide whether it's dropping a usable location, or doing the right thing with line 0.
>
> Because of below code Global-isel is creating line zero and This is still looks incorrect to me,Because it is not solving jump behavior but it is creating jump behavior by adding line zero.

It's solving jump behavior because the location may have been pulled from some other basic block - so preserving its location would be problematic.

But it's possible that the right solution is to drop the location, rather than use line zero here. Perhaps this code in GlobalISel should be using updateLocationAfterHoist ( https://reviews.llvm.org/D85670 ) @vsk @aprantl

> llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2183
>
>   // We only emit constants into the entry block from here. To prevent jumpy
>    // debug behaviour set the line to 0.
>    if (const DebugLoc &DL = Inst.getDebugLoc())
>      EntryBuilder->setDebugLoc(
>          DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
>    else
>      EntryBuilder->setDebugLoc(DebugLoc());




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

https://reviews.llvm.org/D83468