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

Andrew Burgess
2020-07-20T16:57:23Z

* 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