Re: [PATCH 1/3] Check function is GC'ed

Doug Evans
2014-09-18T16:42:09Z

Yao Qi writes:
> Doug Evans <dje@google.com> writes:
>
> Thanks for the review, Doug.
>
> > > /* NOTE: pst->dirname is DW_AT_comp_dir (if present). */
> > > - dwarf_decode_lines (lh, pst->dirname, cu, pst);
> > > + dwarf_decode_lines (lh, pst->dirname, cu, pst, lowpc);
> >
> > We don't really need a "lowpc" argument to dwarf2_build_include_psymtabs.
> > Replace that with:
> >
> > dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
> >
> > and remove the lowpc argument to dwarf2_build_include_psymtabs.
> >
>
> That is fine to me. The reason I add "lowpc" argument to
> dwarf2_build_include_psymtabs is that I'd like to keep the similarity
> between dwarf2_build_include_psymtabs and handle_DW_AT_stmt_list.
>
> > > @@ -17252,7 +17255,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
> > >
> > > static void
> > > dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
> > > - struct dwarf2_cu *cu, const int decode_for_pst_p)
> > > + struct dwarf2_cu *cu, const int decode_for_pst_p,
> > > + CORE_ADDR lowpc)
> > > {
> > > const gdb_byte *line_ptr, *extended_end;
> > > const gdb_byte *line_end;
> > > @@ -17375,7 +17379,9 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
> > > case DW_LNE_set_address:
> > > address = read_address (abfd, line_ptr, cu, &bytes_read);
> > >
> > > - if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
> > > + if (address == 0
> > > + && (!dwarf2_per_objfile->has_section_at_zero
> > > + || lowpc > address))
> >
> > I'm trying to decide whether to keep the
> > "!dwarf2_per_objfile->has_section_at_zero"
> > test here. It feels wrong to keep it: What does it matter
> > whether any section has address zero? It was only used as a proxy
> > for a better test. Here we know we have an address
> > that is outside the CU in question, which is a far more specific test
> > than just checking whether any section is at address zero.
> > But maybe I'm missing something.
>
> I thought about this when I wrote the patch. I keep it in order to
> preserve GDB's behaviour. It should be OK to remove it.
>
> >
> > One could even argue that the "address == 0" test is also
> > now superfluous.
> >
> > IOW, I'm wondering if we could just write the following here:
> >
> > if (lowpc > address)
> >
> > But if we write it that way then the code no longer readily expresses
> > what we're trying to do here which is handle the specific case expressed by
> > the comment that follows: "This line table is for a function which has been
> > GCd by the linker." Instead we'd be now testing for a more general
> > case which would include bad debug info.
> >
> > What do you think?
> >
> > I'm leaning towards not changing things too much in this patch
> > and only handling GC'd functions. Therefore I'm leaning towards
> > something like the following:
> >
> > /* If address < lowpc then it's not a usable value, it's
> > outside the pc range of the CU. However, we restrict
> > the test to only address values of zero to preserve
> > GDB's previous behaviour which is to handle the specific
> > case of a function being GC'd by the linker. */
> > if (address == 0 && address < lowpc)
>
> I agree with you on this. This patch is to fix a GDB bug when a
> function is GC'ed by linker, we can concentrate on this at first. We
> can remove "address == 0" if it fixes some bugs we find in the future.
>
> Patch below is updated to address your comments above.
>
> --
> Yao ( )
>
> From: Yao Qi <yao@codesourcery.com>
> Subject: [PATCH] Check function is GC'ed
>
> I see the following fail on arm-none-eabi target,
>
> (gdb) b 24^M
> Breakpoint 1 at 0x4: file
> ../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
> line 24.^M
> (gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24
>
> Currently, we are using flag has_section_at_zero to determine whether
> address zero in debug info means the corresponding code has been
> GC'ed, like this:
>
> case DW_LNE_set_address:
> address = read_address (abfd, line_ptr, cu, &bytes_read);
>
> if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
> {
> /* This line table is for a function which has been
> GCd by the linker. Ignore it. PR gdb/12528 */
>
> However, this is incorrect on some bare metal targets, as .text
> section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
> is true. If a function is GC'ed by linker, the address is zero. GDB
> thinks address zero is a function's address rather than this function
> is GC'ed.
>
> In this patch, we choose 'lowpc' got in read_file_scope to check
> whether 'lowpc' is greater than zero. If it isn't, address zero really
> means the function is GC'ed. In this patch, we pass 'lowpc' in
> read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
> and to dwarf_decode_lines_1 finally.
>
> This patch fixes the fail above. This patch also covers the path that
> partial symbol isn't used, which is tested by starting gdb with
> --readnow option.
>
> It is regression tested on x86-linux with
> target_board=dwarf4-gdb-index, and arm-none-eabi. OK to apply?
>
> gdb:
>
> 2014-09-16 Yao Qi <yao@codesourcery.com>
>
> * dwarf2read.c (dwarf_decode_lines): Update declaration.
> (handle_DW_AT_stmt_list): Add argument 'lowpc'. Update
> comments. Callers update.
> (dwarf_decode_lines): Likewise.
> (dwarf_decode_lines_1): Add argument 'lowpc'. Update
> comments. Skip the line table if 'lowpc' is greater than
> 'address'. Don't check
> dwarf2_per_objfile->has_section_at_zero.
>
> gdb/testsuite:
>
> 2014-09-16 Yao Qi <yao@codesourcery.com>
>
> * gdb.base/break-on-linker-gcd-function.exp: Move test into new
> proc set_breakpoint_on_gcd_function. Invoke
> set_breakpoint_on_gcd_function. Restart GDB with --readnow and
> invoke set_breakpoint_on_gcd_function again.

LGTM.