From patchwork Tue Sep 16 02:36:31 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 2853 Received: (qmail 12620 invoked by alias); 16 Sep 2014 02:40:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 12610 invoked by uid 89); 16 Sep 2014 02:40:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 16 Sep 2014 02:40:48 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1XTihB-0003n6-1C from Yao_Qi@mentor.com ; Mon, 15 Sep 2014 19:40:45 -0700 Received: from GreenOnly (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.3.181.6; Mon, 15 Sep 2014 19:40:44 -0700 From: Yao Qi To: Doug Evans CC: Subject: Re: [PATCH 1/3] Check function is GC'ed References: <53D8A264.1050103@codesourcery.com> <1408609338-17561-1-git-send-email-yao@codesourcery.com> <21495.32892.341399.802579@ruffy2.mtv.corp.google.com> <87sikggadr.fsf@codesourcery.com> <21527.13753.980949.662368@ruffy2.mtv.corp.google.com> Date: Tue, 16 Sep 2014 10:36:31 +0800 In-Reply-To: <21527.13753.980949.662368@ruffy2.mtv.corp.google.com> (Doug Evans's message of "Mon, 15 Sep 2014 11:53:45 -0700") Message-ID: <87ioko1ec0.fsf@codesourcery.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Doug Evans 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. diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index be32309..9d0ee13 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -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) { diff --git a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp index e593b51..b8de1cd 100644 --- a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp +++ b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp @@ -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 +}