Re: [PATCH] gdb: Don't hard code 0 as end marker in GDB's line table

[Simon Marchi <simark@simark.ca>]

On 2020-07-20 8:55 a.m., Andrew Burgess wrote:
> Currently GDB hard-codes the use of 0 as the end of sequence marker in
> its line tables.  After this commit GDB now uses a named
> constant (linetable_entry::end_marker) as the end of sequence marker.
> The value of this constant is -1, not 0.  This change was made in
> order to aid fixing bug PR gdb/26243.
> 
> Bug PR gdb/26243 is about allowing line number 0 to be used to
> indicate a program address that has no corresponding source line (this
> use is specified in the DWARF standard).  Currently GDB can't use line
> number 0 as this line number is hard coded as the end of sequence
> marker, but, after this commit line number 0 no longer has any special
> meaning.
> 
> This commit does not fix PR gdb/26243, but it is step towards allowing
> that issue to be fixed.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/26243
> 	* buildsym.c (buildsym_compunit::record_line): Add an assert for
> 	the incoming line number.  Update comments to not mention 0
> 	specifically.  Update to check for linetable_entry::end_marker
> 	rather than 0.
> 	* disasm.c (do_mixed_source_and_assembly_deprecated): Check for
> 	linetable_entry::end_marker not 0.
> 	(do_mixed_source_and_assembly): Likewise.
> 	* dwarf2/read.c (dwarf_finish_line): Pass
> 	linetable_entry::end_marker not 0.
> 	* mdebugread.c (add_line): Set is_stmt field.
> 	* python/py-linetable.c (ltpy_get_all_source_lines): Check for
> 	linetable_entry::end_marker not 0, update comments.
> 	(ltpy_iternext): Likewise.
> 	* record-btrace.c (btrace_find_line_range): Likewise.
> 	* symmisc.c (maintenance_print_one_line_table): Likewise.
> 	* symtab.c (find_pc_sect_line): Likewise.
> 	(find_line_symtab): Likewise.
> 	(skip_prologue_using_sal): Likewise.
> 	* symtab.h (linetable_entry::end_marker): New const static member
> 	variable.  Add a static assert for this field.
> 	* xcoffread.c (arrange_linetable): Check for
> 	linetable_entry::end_marker not 0.

I think the idea is good.  But we need to make sure to audit all the places
that use linetable_entry::line to see if they now need to treat the value 0
specially.  One case that came to mind while reading your patch is
"disassemble /s".  Here's an example using bug 26243's reproducer, showing
the end of "disassemble /s tree_insert".  The instruction at 0x40135f maps
to line 0.

This is with GDB 8.3:

---
38            else
39              root->right = make_node(val);
   0x000000000040134a <+138>:   mov    -0xc(%rbp),%edi
   0x000000000040134d <+141>:   callq  0x401250 <make_node(int)>
   0x0000000000401352 <+146>:   mov    -0x8(%rbp),%rcx
   0x0000000000401356 <+150>:   mov    %rax,0x10(%rcx)

40          }
   0x000000000040135a <+154>:   jmpq   0x40135f <tree_insert(node*, int)+159>
   0x000000000040135f <+159>:   jmpq   0x401364 <tree_insert(node*, int)+164>

41      }
   0x0000000000401364 <+164>:   add    $0x10,%rsp
   0x0000000000401368 <+168>:   pop    %rbp
   0x0000000000401369 <+169>:   retq
---

This made 0x40135f appear as if it was part of the previous line.

This is with GDB master:

---
38            else
39              root->right = make_node(val);
   0x000000000040134a <+138>:   mov    -0xc(%rbp),%edi
   0x000000000040134d <+141>:   call   0x401250 <_Z9make_nodei>
   0x0000000000401352 <+146>:   mov    -0x8(%rbp),%rcx
   0x0000000000401356 <+150>:   mov    %rax,0x10(%rcx)

40          }
   0x000000000040135a <+154>:   jmp    0x40135f <_Z11tree_insertP4nodei+159>

unknown:
--- no source info for this pc ---
   0x000000000040135f <+159>:   jmp    0x401364 <_Z11tree_insertP4nodei+164>

test.cpp:
26      {
27        if (val < root->id)
28          {
29            if (root->left)
30              tree_insert (root->left, val);
31            else
32              root->left = make_node(val);
33          }
34        else if (val > root->id)
35          {
36            if (root->right)
37              tree_insert (root->right, val);
38            else
39              root->right = make_node(val);
40          }
41      }
   0x0000000000401364 <+164>:   add    $0x10,%rsp
   0x0000000000401368 <+168>:   pop    %rbp
   0x0000000000401369 <+169>:   ret
End of assembler dump.
---

I don't know what happens here, but it doesn't look good.  The "no source info for this pc"
part is ok, but the "unknown:" and source lines 26-40 look wrong.

This is with GDB master + your patch:

---
38            else
39              root->right = make_node(val);
   0x000000000040134a <+138>:   mov    -0xc(%rbp),%edi
   0x000000000040134d <+141>:   call   0x401250 <_Z9make_nodei>
   0x0000000000401352 <+146>:   mov    -0x8(%rbp),%rcx
   0x0000000000401356 <+150>:   mov    %rax,0x10(%rcx)

40          }
   0x000000000040135a <+154>:   jmp    0x40135f <_Z11tree_insertP4nodei+159>

Line number 0 out of range; test.cpp has 80 lines.
---

The disassembly gets interrupted, because the line number is invalid.

In this case, line 0 should probably get handled specially so that we print
the "no source info for this pc" message and keep printing the rest correctly.

So, I think we just need to be careful and look at the possible behavior changes
of all these spots, not just blindly change `line == 0` for `line == end_marker`.

Simon