[Debuginfo] Fix for PR46653

[Jac1494]

Summary

$ cat check.c
#include<stdio.h>
int main()
{
   printf("Hello\n");
   return 0;
}
$clang  -target arm64 -g -c check.c
$./llvm-dwarfdump --debug-line check.o

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      4      0      1   0             0  is_stmt
0x0000000000000014      0      0      1   0             0  is_stmt prologue_end
0x0000000000000020      5      4      1   0             0  is_stmt
0x0000000000000028      6      4      1   0             0  is_stmt
0x0000000000000038      6      4      1   0             0  is_stmt end_sequence

In LLVM geneated executables prologue_end in line table is set to zero ,because of that
when we put break point on main it will show line number of main function location at line zero incorrectly.

And in gcc it is working fine. This issue is specific with Global-isel.

[dstenb]

Could you please add a paragraph to the summary that explains why this change is done?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1987–1989 ↗

(On Diff #276676)

AFAICT, this comment is no longer valid.

llvm/test/DebugInfo/X86/dbg-prolog-end.ll

29 ↗

(On Diff #276676)

Since this test is called "dbg-prolog-end", perhaps this should still have a CHECK that verifies the position where prologue_end is emitted on?

[probinson]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1990 ↗

(On Diff #276676)

It looks like this change will suppress emitting an explicit line-0 location. We definitely want to emit line-0 in many cases, so this patch is going too far.

[Jac1494]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1990 ↗

(On Diff #276676)

Can you please provide me testcase where line entry "0" is required..?
I couldn't find the use case.

[probinson]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1990 ↗

(On Diff #276676)

For example, the tail-merge.ll test that was modified. It should not be modified.
When merging identical instructions that were originally attributed to different source locations, we cannot preserve both source attributions because the debug-info formats do not allow it. Preserving either one of the original source locations provides the correct answer only 50% of the time, which is bad both for the debugging experience and for sample-based PGO.
Therefore, we intentionally assign the merged instruction to line 0, which in DWARF means "no definitive source location." CodeView has a similar concept although it is encoded differently.

[probinson]

According to https://bugs.llvm.org/show_bug.cgi?id=46653#c3 there is no problem on x86-64, suggesting the fix is more likely to be in the AArch64 target instead of in generic code. It's not uncommon for passes to lose source locations, or fail to add them when they should, so that is more likely to be the cause of the AArch64 issue.

[Jac1494]

Issue is with Global-isel and specific to AAarch64.

[aemerson]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

2183

So...you deleted the previous code here to prevent jumpy debug behavior. How does the new code address that problem?

[aemerson]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

2183

On further thought, I think we can probably do better here in general to preserve more debug info. SelectionDAG doesn't drop or zero the loc for the global value reference, so we should be able to keep it too.

[aemerson]

@aprantl Adrian, what do you think? I thought that using line 0 was fine when there wasn't a "correct" loc we could use.

[Jac1494]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

2183

I have tried below testcase which is mention in PR 40887.

int var1;           // 1
int var2;           // 2
int main() {        // 3
  if (var1 == 1) {  // 4
    var2 = 2;       // 5
  }                 // 6
  return 0;         // 7
}

When we set break point to main,so control goes from line 1-> 4 ->1 ->7(when global-isel),But with SelectionDAg/fastisel control goes from line 4-> 7 which seems correct to me ,since line 1 is not executable statement.

This behaviour is prevalent before and after this patch. This patch don't fix this problem.

[Jac1494]

ping?

[aprantl]

I'm skeptical of this patch. Removing the debug location entirely causes the instruction to "inherit" the location of the instruction before it, but it also completely throws away inline information for the instruction, which can be anywhere from misleading to asserting in the LLVM verifier (if the instruction is a call).

Can you post the output of llvm-dwarfdump --debug-line before and after the patch?

I fear that this patch only "fixes" the observable behavior by accident and the real problem you're after is somewhere else.

[Jac1494]

Hi @aprantl , Thank you for reply.

Can you post the output of llvm-dwarfdump --debug-line before and after the patch?

Test.c:-
====
int var1;           // 1
int var2;           // 2
int main() {        // 3
  if (var1 == 1) {  // 4
    var2 = 2;       // 5
  }                 // 6
  return 0;         // 7
}

Before patch:-
=============

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      3      0      1   0             0  is_stmt
0x0000000000000008      0      0      1   0             0  is_stmt prologue_end
0x000000000000000c      4      7      1   0             0  is_stmt
0x0000000000000010      4      7      1   0             0
0x0000000000000018      0      0      1   0             0
0x0000000000000020      5     10      1   0             0  is_stmt
0x0000000000000024      0     10      1   0             0
0x0000000000000028      7      3      1   0             0  is_stmt
0x0000000000000034      7      3      1   0             0  is_stmt end_sequence

After patch:-
=============
Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      3      0      1   0             0  is_stmt
0x000000000000000c      4      7      1   0             0  is_stmt prologue_end
0x0000000000000010      4      7      1   0             0
0x0000000000000018      0      7      1   0             0
0x0000000000000020      5     10      1   0             0  is_stmt
0x0000000000000024      0     10      1   0             0
0x0000000000000028      7      3      1   0             0  is_stmt
0x0000000000000034      7      3      1   0             0  is_stmt end_sequence

[aprantl]

Thanks for posting the line table. It appears to confirm my suspicion that this patch merely hides the issue, but it isn't correct. To be sure it may be possible to preserve the debug location in this function, but implicitly setting it to the location of the instruction before it, is not the right thing to do. It may have the desired effect in your example, but it't not generally correct.

[Jac1494]

In D83468#2177491, @aprantl wrote:

Thanks for posting the line table. It appears to confirm my suspicion that this patch merely hides the issue, but it isn't correct. To be sure it may be possible to preserve the debug location in this function, but implicitly setting it to the location of the instruction before it, is not the right thing to do. It may have the desired effect in your example, but it't not generally correct.

@aprantl Thank you for comment . Line llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2187

EntryBuilder->setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));

is setting Debug location for Line number zero , because of that AsmPrinter is getting debug location for line number zero and it is adding this entry into debug_line table . And this case is not there with SelectionDAG/FastIsel so it is working fine.

 llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
...
  if (DL == PrevInstLoc) { // Checking for DL(DebugLoc) is give location which is zero in case of SelectionDAG/FastIsel
    // If we have an ongoing unspecified location, nothing to do here.
    if (!DL)            //SelectionAG/FastIsel will return 
      return;
    // We have an explicit location, same as the previous location.
    // But we might be coming back to it after a line 0 record.
    if (LastAsmLine == 0 && DL.getLine() != 0) {
      // Reinstate the source location but not marked as a statement.
      const MDNode *Scope = DL.getScope();
      recordSourceLine(DL.getLine(), DL.getCol(), Scope, /*Flags=*/0);
    }
    return;
  }
 ...

And If we use gnu-as with clang (option -no-integrated-as) than it is remove line number zero entry from debug_line table.
So what is proper place to fix this issue compiler side or builtin assembler...?

[Jac1494]

Issue is with Global-isel , So trying to handle issue in Legalizer. Also tested debugging behavior and it is fine.

[Jac1494]

PING?

[aprantl]

In D83468#2207667, @Jac1494 wrote:

In D83468#2177491, @aprantl wrote:

Thanks for posting the line table. It appears to confirm my suspicion that this patch merely hides the issue, but it isn't correct. To be sure it may be possible to preserve the debug location in this function, but implicitly setting it to the location of the instruction before it, is not the right thing to do. It may have the desired effect in your example, but it't not generally correct.

@aprantl Thank you for comment . Line llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2187

EntryBuilder->setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));

is setting Debug location for Line number zero , because of that AsmPrinter is getting debug location for line number zero and it is adding this entry into debug_line table .

Yes, and that seems to be the correct behavior.

I think I haven't quit understood what your goal is. Are you trying to avoid having the prologue end at line 0?

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

107 ↗

(On Diff #284871)

This also looks wrong. We would be inheriting a random previous debug location for all instructions created by MIRBuilder if we did this?

[Jac1494]

I think I haven't quit understood what your goal is. Are you trying to avoid having the prologue end at line 0?

Yes, Also other line 0 too which is making incorrect debugging experience.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

107 ↗

(On Diff #284871)

Yes, I have also check the debugging experience and now it is looks correct.

[Jac1494]

https://github.com/llvm/llvm-project/commit/32823091c36cfa2b27b717246f15d4f12591e6f4

[Jac1494]

PING??

[aprantl]

If the problem that you want to solve is that the prologue ends at line 0, then massaging earlier passes until the right result happens to come up in one testcase is not going to be very effective. Yes, it may fix the testcase, but that won't last long, and the patch is introducing questionable behavior that will end up causing other problems.

Instead I would recommend looking at the code that decides on were the prologue ends, and either

• sliding the prologue forward to the first non-zero debug location
• forcing a valid line number (perhaps the DSISubprogram's scope line) if the prologue end ends up falling on line 0.

Both of these options are also going to be somewhat risky, but I'd have a better feeling about adding a well-documented heuristic at the place where the problem happens rather than a couple of passes before. If anyone has a better idea, I'm also open to discussing that.

[dblaikie]

In D83468#2221792, @aprantl wrote:

If the problem that you want to solve is that the prologue ends at line 0,

It'd be good to understand more about why/where that's a problem. What kind of problems it causes and for which DWARF consumers.

then massaging earlier passes until the right result happens to come up in one testcase is not going to be very effective. Yes, it may fix the testcase, but that won't last long, and the patch is introducing questionable behavior that will end up causing other problems.

Instead I would recommend looking at the code that decides on were the prologue ends, and either

• sliding the prologue forward to the first non-zero debug location

I'd worry about this skipping over valid debug locations, maybe making parameters that are valid just-after the prologue (as it stands today) not valid (because we push the prologue down past clobbers of those parameters).

• forcing a valid line number (perhaps the DSISubprogram's scope line) if the prologue end ends up falling on line 0.

This seems fairly plausible to me, but again - would be good to better understand the user-visible/original problem too.

Both of these options are also going to be somewhat risky, but I'd have a better feeling about adding a well-documented heuristic at the place where the problem happens rather than a couple of passes before. If anyone has a better idea, I'm also open to discussing that.

[arsenm]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

107 ↗

(On Diff #284871)

Pretty sure this is completely broken. The currently legalizing instruction is the only reasonable choice to set here

[Jac1494]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

107 ↗

(On Diff #284871)

If we don't set debug location here than also in executable debug location entry is added properly , because IRTransaltor is already setting debug location. I have tried test case given below and others test cases too and debugging experience is improved lot after this patch.

int var1;           // 1
int var2;           // 2
int main() {        // 3
  if (var1 == 1) {  // 4
    var2 = 2;       // 5
  }                 // 6
  return 0;         // 7
}

Also if we add debug location here than it is giving incorrect debugging experience because of line number 0 entry added into debugline table which is added from here ,than what is need of setting debug location here (In legalizer)...?

Pretty sure this is completely broken.

Can you please explain more what is completely broken here..?

[arsenm]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

107 ↗

(On Diff #284871)

The IRTranslator added a debug location to the initial instruction. The same debug location needs to be propagated to all instructions emitted when the instruction is legalized, which setting it here accomplishes. This is not setting or changing the debug location of the instruction itself, and only ensures that new instructions derived from it end up with the same location

[Jac1494]

In D83468#2221792, @aprantl wrote:

If the problem that you want to solve is that the prologue ends at line 0, then massaging earlier passes until the right result happens to come up in one testcase is not going to be very effective. Yes, it may fix the testcase, but that won't last long, and the patch is introducing questionable behavior that will end up causing other problems.

Please consider below testcase for further discussion:

 1 /* demo testcase */
 2 int garr[2];
 3 int main() {
 4
 5         if((garr[0] && garr[1])==0){
 6                 garr[0]=1;
 7                 garr[1]=1;
 8         }
 9         else {
10                 garr[0]=2;
11                 garr[1]=2;
12         }
13         printf("%d %d\n",garr[0], garr[1]);
14         return 0;
15 }

Currently there are two problem is there and both are because of line number zero

1. prologue_end with line zero (Because of this when we set brackpoint on function control goes to line zero)

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

When we try to set break point in main than it is showing breakpoint is set at line zero because of prologue_end is at line zero

2. Other debug line zero also creating debugging problem,which mentioned in below case.In this case because of line number zero , control is going to line 1

Breakpoint 1, main () at test.c:0
1       /* demo testcase */
(gdb) n
5               if((garr[0] && garr[1])==0){
(gdb)
1       /* demo testcase */
(gdb)
5               if((garr[0] && garr[1])==0){
(gdb)
1       /* demo testcase */
(gdb)
6                       garr[0]=1;
(gdb)
7                       garr[1]=1;
(gdb)
8               }
(gdb)
1       /* demo testcase */
(gdb)
13              printf("%d %d\n",garr[0], garr[1]);
(gdb)
1       /* demo testcase */
(gdb)
13              printf("%d %d\n",garr[0], garr[1]);
(gdb)
1 1
14              return 0;
(gdb)

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      3      0      1   0             0  is_stmt
0x000000000000000c      0      0      1   0             0  is_stmt prologue_end
0x000000000000001c      5     13      1   0             0  is_stmt
0x000000000000002c      5     21      1   0             0
0x0000000000000030      0     21      1   0             0
0x0000000000000034      5     24      1   0             0
0x0000000000000038      5     21      1   0             0
0x0000000000000044      0     21      1   0             0
0x0000000000000048      5     12      1   0             0
0x000000000000004c      0     12      1   0             0
0x0000000000000054      6     24      1   0             0  is_stmt
0x000000000000005c      7     24      1   0             0  is_stmt
0x0000000000000060      8      9      1   0             0  is_stmt
0x0000000000000064      0      9      1   0             0
0x000000000000006c     10     24      1   0             0  is_stmt
0x0000000000000074     11     24      1   0             0  is_stmt
0x0000000000000078      0      0      1   0             0
0x000000000000007c     13     26      1   0             0  is_stmt
0x0000000000000084     13     35      1   0             0
0x0000000000000088      0      0      1   0             0
0x0000000000000090     13      9      1   0             0
0x0000000000000098     14      9      1   0             0  is_stmt
0x00000000000000a8     14      9      1   0             0  is_stmt end_sequence

This issue is with Global-isel and this line number is set in IRTranslator and Legalizer also.And I have suggested fix in IRTranslator and Legalizer pass previously but it was rejected.

Is above understanding of the issue matches with your understanding ..?

@aprantl , @arsenm Could you please suggest where this should be fixed ?? if this is not correct place to fix this issue , In other common passes of Global-isel and Selection-DAG or AsmPrinter...?

[arsenm]

In D83468#2224469, @Jac1494 wrote:

This issue is with Global-isel and this line number is set in IRTranslator and Legalizer also.And I have suggested fix in IRTranslator and Legalizer pass previously but it was rejected.

Is above understanding of the issue matches with your understanding ..?

@aprantl , @arsenm Could you please suggest where this should be fixed ?? if this is not correct place to fix this issue , In other common passes of Global-isel and Selection-DAG or AsmPrinter...?

You're describing this in terms that are too high level. What is the codegen problem here? The IRTranslator and Legalizer do not set the line number, they're supposed to just forward the line number it already has. How/why is this a codegen problem? Why is the debug location they're starting with not correct?

llvm/test/DebugInfo/debugline-no-prologue_end.ll

3

This check is far too fragile. This should use positive checks

23–28

Don't need most of these attributes or metadata

[Jac1494]

The IRTranslator and Legalizer do not set the line number, they're supposed to just forward the line number it already has.

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());

@arsenm Seems like it is creating not forwarding...?

[arsenm]

In D83468#2224496, @Jac1494 wrote:

The IRTranslator and Legalizer do not set the line number, they're supposed to just forward the line number it already has.

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());

@arsenm Seems like it is creating not forwarding...?

Look above that. It's set on the CurBuilder. The debug location is dropped for special things that are inserted in the entry block from another position. It's odd that this is cleared for every instruction translated though

[aprantl]

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

Agreed that that is not a helpful end-user experience.

Other debug line zero also creating debugging problem,which mentioned in below case.In this case because of line number zero , control is going to line 1

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

[dblaikie]

In D83468#2224754, @aprantl wrote:

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

Agreed that that is not a helpful end-user experience.

How does LLDB handle this? Might also be consumer-fixable.

Other debug line zero also creating debugging problem,which mentioned in below case.In this case because of line number zero , control is going to line 1

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

[aprantl]

In D83468#2224774, @dblaikie wrote:

In D83468#2224754, @aprantl wrote:

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

Agreed that that is not a helpful end-user experience.

How does LLDB handle this? Might also be consumer-fixable.

My understanding is that like with a regular breakpoint it would also slide the prologue-end breakpoint forward to the first non-zero line number. I'm not sure what happens if it's line 0 all the way to the end of the basic block — I think it would break on line 0 in that case. @jingham can correct me.

[Jac1494]

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

@aprantl I understand line zero meaning in DWARF standard.
But could you please explain that why this behavior is there with only with Global-isel enabled and NOT with SelectionDAG/FastIsel...?
I mean when selection/fast-isel is enabled this not seen when using GDB. That suggest bug in global-isel not in GDB...?

[jingham]

In D83468#2226461, @aprantl wrote:

In D83468#2224774, @dblaikie wrote:

In D83468#2224754, @aprantl wrote:

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

Agreed that that is not a helpful end-user experience.

How does LLDB handle this? Might also be consumer-fixable.

My understanding is that like with a regular breakpoint it would also slide the prologue-end breakpoint forward to the first non-zero line number. I'm not sure what happens if it's line 0 all the way to the end of the basic block — I think it would break on line 0 in that case. @jingham can correct me.

This is done in Function::GetPrologueByteSize. The computation there is pretty aggressive, and so long as the first non-zero line is before the end of the function we'll slide it to there. So the only case where we would set the breakpoint on line 0 is if the whole function has line-number 0.

I have one bug where the branching part of a switch statement that is the first line of a function got absorbed by line number 0 (most likely a debug info bug) so that the breakpoint got slid to the first case of the switch. At some point I need to refine the prologue skipping code so that it doesn't skip past branches. This isn't 100% trivial since there are branches that naturally occur in prologues like the "branch to next instruction" trick which some systems use in the prologue to read out the pc. And some architectures have "set up all the FP register functions" that get called in prologues as well.

Anyway, that stream of consciousness comment is off-topic. To answer Adrian's question, we push the prologue to the first non-zero line as long as there is one in the function.

[aemerson]

In D83468#2226466, @Jac1494 wrote:

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

@aprantl I understand line zero meaning in DWARF standard.
But could you please explain that why this behavior is there with only with Global-isel enabled and NOT with SelectionDAG/FastIsel...?
I mean when selection/fast-isel is enabled this not seen when using GDB. That suggest bug in global-isel not in GDB...?

GlobalISel is a different instruction selection framework to SelectionDAG. Therefore, it can make different decisions and generate different code. That does not necessarily imply that there's a bug in GlobalISel, it may just be exposing an existing issue.

[dblaikie]

In D83468#2226596, @aemerson wrote:

In D83468#2226466, @Jac1494 wrote:

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

@aprantl I understand line zero meaning in DWARF standard.
But could you please explain that why this behavior is there with only with Global-isel enabled and NOT with SelectionDAG/FastIsel...?
I mean when selection/fast-isel is enabled this not seen when using GDB. That suggest bug in global-isel not in GDB...?

GlobalISel is a different instruction selection framework to SelectionDAG. Therefore, it can make different decisions and generate different code. That does not necessarily imply that there's a bug in GlobalISel, it may just be exposing an existing issue.

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.

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.

And, in any case, at the moment it's intentional that LLVM can produce line 0 both at the start of a function/end of the prologue and elsewhere. So if some debuggers have problems with line zero - I think at least at a first pass, it's worth pushing back a bit on changing LLVM and pushing towards changing the DWARF Consumers to handle line zero better (even if these particular line zeros turn out to be bugs/missed location opportunities - they won't be the only ones/not all of them will be bugs). If someone came back and told me GDB would not accept patches to improve line zero handling - then I think we could have a discussion about how to modify LLVM's output under a flag (-ggdb or others) to be more GDB-compatible.

[aemerson]

In D83468#2226705, @dblaikie wrote:

In D83468#2226596, @aemerson wrote:

In D83468#2226466, @Jac1494 wrote:

I would consider this to be more of a bug in GDB. Line 0 has a well-defined meaning in DWARF and stands for "no source location corresponds to this code". LLDB, for example, interprets line 0 as an indicator to skip over instructions and automatically steps over them when single-stepping through a program, since line 0 is not a meaningful place to stop. It might be worth talking to the GDB developers and ask them what they think about adding a similar feature to their handling of line 0. DWARF doesn't prescribe that that's how debuggers should behave, but I think it's a reasonable interpretation.

@aprantl I understand line zero meaning in DWARF standard.
But could you please explain that why this behavior is there with only with Global-isel enabled and NOT with SelectionDAG/FastIsel...?
I mean when selection/fast-isel is enabled this not seen when using GDB. That suggest bug in global-isel not in GDB...?

GlobalISel is a different instruction selection framework to SelectionDAG. Therefore, it can make different decisions and generate different code. That does not necessarily imply that there's a bug in GlobalISel, it may just be exposing an existing issue.

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.

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.

And, in any case, at the moment it's intentional that LLVM can produce line 0 both at the start of a function/end of the prologue and elsewhere. So if some debuggers have problems with line zero - I think at least at a first pass, it's worth pushing back a bit on changing LLVM and pushing towards changing the DWARF Consumers to handle line zero better (even if these particular line zeros turn out to be bugs/missed location opportunities - they won't be the only ones/not all of them will be bugs). If someone came back and told me GDB would not accept patches to improve line zero handling - then I think we could have a discussion about how to modify LLVM's output under a flag (-ggdb or others) to be more GDB-compatible.

In this case. the reason we use line 0 for some instructions is that in GlobalISel, we essentially CSE constant values and generate them in the entry block, and those constants may be re-used by multiple instructions in different basic blocks. The choice for us is whether to use no line info at all, or to use line 0.

@Jac1494 I think you need to ask GDB if this is something they can address on their side.

[Jac1494]

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.But this options is not fully working for Global-isel because of incorrect line zeros.

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.

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());

[dblaikie]

In 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());

[Jac1494]

In D83468#2226884, @dblaikie wrote:

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());

void Instruction::updateLocationAfterHoist() {
  const DebugLoc &DL = getDebugLoc();
  if (!DL)
    return;

Code Snippet from link (https://reviews.llvm.org/D85670) is not setting the line number at all.
And as seen/observed ,we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero).

I'm sharing the code snippet to avoid unnecessary revision hindering the actual discussion.Is this Okey ? Or should I revise ?

[aemerson]

In D83468#2236637, @Jac1494 wrote:

In D83468#2226884, @dblaikie wrote:

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());

void Instruction::updateLocationAfterHoist() {
  const DebugLoc &DL = getDebugLoc();
  if (!DL)
    return;

Code Snippet from link (https://reviews.llvm.org/D85670) is not setting the line number at all.
And as seen/observed ,we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero).

I'm sharing the code snippet to avoid unnecessary revision hindering the actual discussion.Is this Okey ? Or should I revise ?

That snippet is just returning if there was no debug info at all, there's a ! in the if condition.

Have you ruled this out as a gdb bug/possible improvement?

[Jac1494]

In D83468#2236798, @aemerson wrote:

In D83468#2236637, @Jac1494 wrote:

void Instruction::updateLocationAfterHoist() {

const DebugLoc &DL = getDebugLoc();
if (!DL)
  return;

Code Snippet from link (https://reviews.llvm.org/D85670) is not setting the line number at all.
And as seen/observed ,we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero).

I'm sharing the code snippet to avoid unnecessary revision hindering the actual discussion.Is this Okey ? Or should I revise ?

That snippet is just returning if there was no debug info at all, there's a ! in the if condition.

Sorry for misunderstanding , I am also trying to address no debug location.
snippet shared above suggest the removal of this snippet from IRTransaltor.cpp

// 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());

Have you ruled this out as a gdb bug/possible improvement?

Yes , Because Arm64 with selectionDAG works fine. and we can also prevent jumpy behavior by skipping the line number(By not setting to line number zero), this way we can correct Global-isel behavior too.
+ I am also writing about this behavior to gdb mailing list.

[Jac1494]

Hi All,

This issue is fixed in gdb (https://sourceware.org/bugzilla/show_bug.cgi?id=26243).

Thank you so much @aprantl , @probinson ,@dblaikie , @aemerson ,@arsenm, @vsk ,@dstenb , @JDevlieghere for having a good discussion.
Abandoning this revision.

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

   if (Offsets && !Offsets->empty())
     Offsets->clear();
   computeValueLLTs(*DL, *V.getType(), SplitTys, Offsets);
   return SplitTys.size() > 1;
 }
 
 bool IRTranslator::translate(const Instruction &Inst) {
   CurBuilder->setDebugLoc(Inst.getDebugLoc());
-  // 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());
+  EntryBuilder->setDebugLoc(DebugLoc());
 
   auto &TLI = *MF->getSubtarget().getTargetLowering();
   if (TLI.fallBackToDAGISel(Inst))
     return false;
 
   switch (Inst.getOpcode()) {
 #define HANDLE_INST(NUM, OPCODE, CLASS)                                        \
   case Instruction::OPCODE:                                                    \

llvm/test/CodeGen/AArch64/GlobalISel/constant-dbg-loc.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc -mtriple aarch64 -O0 -stop-after=irtranslator -global-isel -verify-machineinstrs %s -o - 2>&1 | FileCheck --match-full-lines %s
 target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
 target triple = "arm64-apple-ios5.0.0"
 
 @var1 = common global i32 0, align 4, !dbg !0
 @var2 = common global i32 0, align 4, !dbg !6
 
 ; We check here that the G_GLOBAL_VALUE has a debug loc with line 0.
 define i32 @main() #0 !dbg !14 {
   ; CHECK-LABEL: name: main
   ; CHECK: bb.1.entry:
   ; CHECK:   successors: %bb.2(0x40000000), %bb.3(0x40000000)
   ; CHECK:   [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0
-  ; CHECK:   [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var1, debug-location !DILocation(line: 0, scope: !18)
+  ; CHECK:   [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var1
   ; CHECK:   [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 1
   ; CHECK:   [[C2:%[0-9]+]]:_(s32) = G_CONSTANT i32 2
-  ; CHECK:   [[GV1:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var2, debug-location !DILocation(line: 0, scope: !22)
+  ; CHECK:   [[GV1:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var2
   ; CHECK:   [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0.retval
   ; CHECK:   G_STORE [[C]](s32), [[FRAME_INDEX]](p0) :: (store 4 into %ir.retval)
   ; CHECK:   [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[GV]](p0), debug-location !17 :: (dereferenceable load 4 from @var1)
   ; CHECK:   [[ICMP:%[0-9]+]]:_(s1) = G_ICMP intpred(eq), [[LOAD]](s32), [[C1]], debug-location !19
   ; CHECK:   G_BRCOND [[ICMP]](s1), %bb.2, debug-location !20
   ; CHECK:   G_BR %bb.3, debug-location !20
   ; CHECK: bb.2.if.then:
   ; CHECK:   successors: %bb.3(0x80000000)

llvm/test/DebugInfo/debugline-no-prologue_end.ll

+; RUN: llc -march=arm64 -filetype=obj -O0 %s -o - | llvm-dwarfdump -debug-line - | FileCheck %s
+; CHECK-NOT: 0x{{.*}} 0 0 1 0 is_stmt prologue_end
+
+; ModuleID = 'debugline-no-prologue_end.c'
+source_filename = "debugline-no-prologue_end.c"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "arm64"
+
+@.str = private unnamed_addr constant [7 x i8] c"Hello\0A\00", align 1
+
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @main() #0 !dbg !7 {
+entry:
+  %retval = alloca i32, align 4
+  store i32 0, i32* %retval, align 4
+  %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i64 0, i64 0)), !dbg !11
+  ret i32 0, !dbg !12
+}
+
+declare dso_local i32 @printf(i8*, ...) #1
+
+attributes #0 = { noinline nounwind optnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="non-leaf" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="non-leaf" "less-precise-fpmad"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 (https://github.com/llvm/llvm-project.git bd87c6bce1c30cc089ffdea5e0f3cf5407ed37c5)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "debugline-no-prologue_end.c", directory: "/home/bft/Jaydeep/llvm_all/debug/llvm-project/build/bin")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project.git bd87c6bce1c30cc089ffdea5e0f3cf5407ed37c5)"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 2, type: !8, scopeLine: 3, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocation(line: 4, column: 4, scope: !7)
+!12 = !DILocation(line: 5, column: 4, scope: !7)