[1/3] Check function is GC'ed
Commit Message
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.
Comments
Doug Evans <dje@google.com> writes:
> > 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.
Thanks for the review. Patch is pushed in.
@@ -1512,7 +1512,8 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
struct dwarf2_cu *cu);
static void dwarf_decode_lines (struct line_header *, const char *,
- struct dwarf2_cu *, struct partial_symtab *);
+ struct dwarf2_cu *, struct partial_symtab *,
+ CORE_ADDR);
static void dwarf2_start_subfile (const char *, const char *, const char *);
@@ -4448,7 +4449,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
return; /* No linetable, so no includes. */
/* 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, pst->textlow);
free_line_header (lh);
}
@@ -8967,11 +8968,12 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
/* Handle DW_AT_stmt_list for a compilation unit.
DIE is the DW_TAG_compile_unit die for CU.
- COMP_DIR is the compilation directory. */
+ COMP_DIR is the compilation directory. LOWPC is passed to
+ dwarf_decode_lines. See dwarf_decode_lines comments about it. */
static void
handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
- const char *comp_dir) /* ARI: editCase function */
+ const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
{
struct attribute *attr;
@@ -8988,7 +8990,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
{
cu->line_header = line_header;
make_cleanup (free_cu_line_header, cu);
- dwarf_decode_lines (line_header, comp_dir, cu, NULL);
+ dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
}
}
}
@@ -9039,7 +9041,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
/* Decode line number information if present. We do this before
processing child DIEs, so that the line header table is available
for DW_AT_decl_file. */
- handle_DW_AT_stmt_list (die, cu, comp_dir);
+ handle_DW_AT_stmt_list (die, cu, comp_dir, lowpc);
/* Process all dies in compilation unit. */
if (die->child != NULL)
@@ -17252,7 +17254,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 +17378,12 @@ 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 < 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)
{
/* This line table is for a function which has been
GCd by the linker. Ignore it. PR gdb/12528 */
@@ -17595,17 +17603,20 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
as the corresponding symtab. Since COMP_DIR is not used in the name of the
symtab we don't use it in the name of the psymtabs we create.
E.g. expand_line_sal requires this when finding psymtabs to expand.
- A good testcase for this is mb-inline.exp. */
+ A good testcase for this is mb-inline.exp.
+
+ LOWPC is the lowest address in CU (or 0 if not known). */
static void
dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
- struct dwarf2_cu *cu, struct partial_symtab *pst)
+ struct dwarf2_cu *cu, struct partial_symtab *pst,
+ CORE_ADDR lowpc)
{
struct objfile *objfile = cu->objfile;
const int decode_for_pst_p = (pst != NULL);
struct subfile *first_subfile = current_subfile;
- dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p);
+ dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
if (decode_for_pst_p)
{
@@ -41,9 +41,23 @@ if {[build_executable_from_specs $testfile.exp $testfile \
clean_restart $testfile
-# Single hex digit
-set xd {[0-9a-f]}
+proc set_breakpoint_on_gcd_function {} {
+ # Single hex digit
+ set xd {[0-9a-f]}
+
+ # This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
+ # but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
+ gdb_test "b [gdb_get_line_number "gdb break here"]" \
+ "Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
+}
+
+set_breakpoint_on_gcd_function
-# This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
-# but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
-gdb_test "b [gdb_get_line_number "gdb break here"]" "Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
+set saved_gdbflags $GDBFLAGS
+set GDBFLAGS "$GDBFLAGS --readnow"
+clean_restart ${testfile}
+set GDBFLAGS $saved_gdbflags
+
+with_test_prefix "readnow" {
+ set_breakpoint_on_gcd_function
+}