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

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2020-07-20 12:01:21 -0400]:

> 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`.

Thanks for pointing this out.  I'll investigate.

I'm confused as to how we could ever have been handling 'line == 0'
as anything other than 'line == end_marker', or how these 'line == 0'
entries were coming from, but I'll start with the example you gave,
and I'm sure it'll make sense.

Thanks,
Andrew