From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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 02:40:00 -0000 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 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00514.txt.bz2 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, st= ruct subfile *subfile, > >=20=20 > > 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 =3D read_address (abfd, line_ptr, cu, &bytes_read); > >=20=20 > > - if (address =3D=3D 0 && !dwarf2_per_objfile->has_section_at_zero) > > + if (address =3D=3D 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 =3D=3D 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 be= en > 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 =3D=3D 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 =3D=3D 0" if it fixes some bugs we find in the future. Patch below is updated to address your comments above. --=20 Yao (=E9=BD=90=E5=B0=A7) From: Yao Qi 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 =3D read_address (abfd, line_ptr, cu, &bytes_read); if (address =3D=3D 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=3Ddwarf4-gdb-index, and arm-none-eabi. OK to apply? gdb: 2014-09-16 Yao Qi * 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 * 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. 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); =20 static void dwarf_decode_lines (struct line_header *, const char *, - struct dwarf2_cu *, struct partial_symtab *); + struct dwarf2_cu *, struct partial_symtab *, + CORE_ADDR); =20 static void dwarf2_start_subfile (const char *, const char *, const char *= ); =20 @@ -4448,7 +4449,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu, return; /* No linetable, so no includes. */ =20 /* 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); =20 free_line_header (lh); } @@ -8967,11 +8968,12 @@ find_file_and_directory (struct die_info *die, stru= ct dwarf2_cu *cu, =20 /* 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. */ =20 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; =20 @@ -8988,7 +8990,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct = dwarf2_cu *cu, { cu->line_header =3D 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); =20 /* Process all dies in compilation unit. */ if (die->child !=3D NULL) @@ -17252,7 +17254,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct = subfile *subfile, =20 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, cons= t char *comp_dir, case DW_LNE_set_address: address =3D read_address (abfd, line_ptr, cu, &bytes_read); =20 - if (address =3D=3D 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 =3D=3D 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, con= st 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). */ =20 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 =3D cu->objfile; const int decode_for_pst_p =3D (pst !=3D NULL); struct subfile *first_subfile =3D current_subfile; =20 - 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); =20 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= \ =20 clean_restart $testfile =20 -# 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 =20 -# 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 +}