From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2762 invoked by alias); 20 Sep 2018 21:51:53 -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 2736 invoked by uid 89); 20 Sep 2018 21:51:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr1-f51.google.com Received: from mail-wr1-f51.google.com (HELO mail-wr1-f51.google.com) (209.85.221.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Sep 2018 21:51:50 +0000 Received: by mail-wr1-f51.google.com with SMTP id v17-v6so10858585wrr.9 for ; Thu, 20 Sep 2018 14:51:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=MVHr+vxbW+Dbn6n53VY3u3JyKwIvTHcqAuDkQju41Ts=; b=UjxN6E83Qx+kXILkoKzLCJcepGLuUNJG64E6po7wg08hM6ekwfVYXyWy6U5hZ3sGAz GUUKg0Ja9LRUTMmsco2UiiDH6tmZhq5e4RKQytewjhE3MfccE7CroCs2DAYwmHHQl6xJ krAfSSxMf6x5cfiHOIueTzDZP8EeWG9TBMXnpMyfi/wjNxrChj1l8mP5kWKf7FWMANZb hLgNlFbe1tcvauCf/Yh+tGzmt/Otwq61agE5KOR90rzFbE+Ze9qdbrKOUAbKcPjvLz6F rzug2Z2WynNl8UHFNJIUBKJzZeHZYDppMtM2ZzjNhwd7R37CcXBTyYho6de6MdGOkc0Z 574Q== Return-Path: Received: from localhost (host81-158-205-250.range81-158.btcentralplus.com. [81.158.205.250]) by smtp.gmail.com with ESMTPSA id u65-v6sm4918789wmd.31.2018.09.20.14.51.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 20 Sep 2018 14:51:47 -0700 (PDT) Date: Thu, 20 Sep 2018 21:51:00 -0000 From: Andrew Burgess To: John Baldwin Cc: Jim Wilson , gdb-patches@sourceware.org, Palmer Dabbelt Subject: Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register. Message-ID: <20180920215146.GW5952@embecosm.com> References: <20180919231950.22634-1-jhb@FreeBSD.org> <20180919231950.22634-3-jhb@FreeBSD.org> <0081bdf8-04cb-f6b7-d80a-d9a878d0a3ab@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: X-Fortune: You attempt things that you do not even plan because of your extreme stupidity. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00742.txt.bz2 This look like a great improvement. I have one issue inline below... * John Baldwin [2018-09-20 13:31:46 -0700]: > On 9/19/18 5:40 PM, John Baldwin wrote: > > Whether or not 'C' is present is a bit more subtle as it isn't something > > you could infer just from a register being present or not. On the other > > hand, for breakpoints, I wonder if a more straightforward approach would > > be to examine the opcode of the existing instruction and use that to > > decide which breakpoint to use? That is, read the first byte of the > > existing instruction and if the low 2 bits indicate a 16-bit instruction > > use C.BREAK and otherwise use BREAK? >=20 > This seems to work for me in my testing (I used 'stepi' to step across a > set of instructions with a mix of regular and 'C' instructions and it > worked correctly). You can use 'set debug riscv breakpoints 1' to see wh= at > it decides each time it sets a breakpoint. I haven't yet tested if this > means I can remove the MISA patch, but I suspect it does as the only > place that needs MISA after this is the riscv_isa_flen() function. >=20 > Author: John Baldwin > Date: Thu Sep 20 10:12:22 2018 -0700 >=20 > Use the existing instruction to determine the RISC-V breakpoint kind. >=20=20=20=20=20 > RISC-V supports instructions of varying lengths. Standard existing > instructions in the base ISA are 4 bytes in length, but the 'C' > extension adds support for compressed, 2 byte instructions. RISC-V > supports two different breakpoint instructions: EBREAK is a 4 byte > instruction in the base ISA, and C.EBREAK is a 2 byte instruction only > available on processors implementing the 'C' extension. Using EBREAK > to set breakpoints on compressed instructions causes problems as the > second half of EBREAK will overwrite the first 2 bytes of the > following instruction breaking other threads in the process if their > PC is the following instruction. Thus, breakpoints on compressed > instructions need to use C.EBREAK instead of EBREAK. >=20=20=20=20=20 > Previously, the riscv architecture checked the MISA register to > determine if the 'C' extension was available. If so, it used C.EBREAK > for all breakpoints. However, the MISA register is not necessarily > available to supervisor mode operating systems. While native targets > could provide a fake MISA register value, this patch instead examines > the existing instruction at a breakpoint target to determine which > breakpoint instruction to use. If the existing instruction is a > compressed instruction, C.EBREAK is used, otherwise EBREAK is used. >=20=20=20=20=20 > gdb/ChangeLog: >=20=20=20=20=20 > * riscv-tdep.c (show_use_compressed_breakpoints): Remove > 'additional_info' and related logic. > (riscv_debug_breakpoints): New variable. > (riscv_breakpoint_kind_from_pc): Use the length of the existi= ng > instruction to determine the breakpoint kind. > (_initialize_riscv_tdep): Add 'set/show debug riscv breakpoin= ts' > flag. Update description of 'set/show riscv > use-compressed-breakpoints' flag. >=20 > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 75032b7a4d..df4561d319 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,14 @@ > +2018-09-20 John Baldwin > + > + * riscv-tdep.c (show_use_compressed_breakpoints): Remove > + 'additional_info' and related logic. > + (riscv_debug_breakpoints): New variable. > + (riscv_breakpoint_kind_from_pc): Use the length of the existing > + instruction to determine the breakpoint kind. > + (_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints' > + flag. Update description of 'set/show riscv > + use-compressed-breakpoints' flag. > + > 2018-09-19 John Baldwin >=20=20 > * Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c. > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > index 4b385ed5da..cd8dbaf9f6 100644 > --- a/gdb/riscv-tdep.c > +++ b/gdb/riscv-tdep.c > @@ -210,20 +210,9 @@ show_use_compressed_breakpoints (struct ui_file *fil= e, int from_tty, > struct cmd_list_element *c, > const char *value) > { > - const char *additional_info; > - struct gdbarch *gdbarch =3D target_gdbarch (); > - > - if (use_compressed_breakpoints =3D=3D AUTO_BOOLEAN_AUTO) > - if (riscv_has_feature (gdbarch, 'C')) > - additional_info =3D _(" (currently on)"); > - else > - additional_info =3D _(" (currently off)"); > - else > - additional_info =3D ""; > - > fprintf_filtered (file, > _("Debugger's use of compressed breakpoints is set " > - "to %s%s.\n"), value, additional_info); > + "to %s.\n"), value); > } >=20=20 > /* The set and show lists for 'set riscv' and 'show riscv' prefixes. */ > @@ -284,6 +273,11 @@ show_riscv_debug_variable (struct ui_file *file, int= from_tty, > c->name, value); > } >=20=20 > +/* When this is set to non-zero debugging information about breakpoint > + kinds will be printed. */ > + > +static unsigned int riscv_debug_breakpoints =3D 0; > + > /* When this is set to non-zero debugging information about inferior cal= ls > will be printed. */ >=20=20 > @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbar= ch, CORE_ADDR *pcptr) > { > if (use_compressed_breakpoints =3D=3D AUTO_BOOLEAN_AUTO) > { > - if (riscv_has_feature (gdbarch, 'C')) > + enum bfd_endian byte_order =3D gdbarch_byte_order_for_code (gdbarc= h); byte_order is unused. > + gdb_byte buf[1]; > + int status; > + > + /* Read the opcode byte to determine the instruction length. */ > + status =3D target_read_memory (*pcptr, buf, 1); This should use target_read_code. I know that we already have some (incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix those later. However, this causes a testsuite regression for gdb.gdb/unittest.exp. You can easily reproduce the issue with: (gdb) maintenance selftest=20 We probably need to add a riscv specific case into disasm-selftest.c:print_one_insn_test, lots of other targets already do. Otherwise, I think this is good. Thanks, Andrew > + if (status) > + memory_error (TARGET_XFER_E_IO, *pcptr); > + > + if (riscv_debug_breakpoints) > + fprintf_unfiltered > + (gdb_stdlog, > + "Using %s for breakpoint at %s (instruction length %d)\n", > + riscv_insn_length (buf[0]) =3D=3D 2 ? "C.EBREAK" : "EBREAK", > + paddress (gdbarch, *pcptr), riscv_insn_length (buf[0])); > + if (riscv_insn_length (buf[0]) =3D=3D 2) > return 2; > else > return 4; > @@ -2945,6 +2954,16 @@ _initialize_riscv_tdep (void) > &showdebugriscvcmdlist, "show debug riscv ", 0, > &showdebuglist); >=20=20 > + add_setshow_zuinteger_cmd ("breakpoints", class_maintenance, > + &riscv_debug_breakpoints, _("\ > +Set riscv breakpoint debugging."), _("\ > +Show riscv breakpoint debugging."), _("\ > +When non-zero, print debugging information for the riscv specific parts\= n\ > +of the breakpoint mechanism."), > + NULL, > + show_riscv_debug_variable, > + &setdebugriscvcmdlist, &showdebugriscvcmdlist); > + > add_setshow_zuinteger_cmd ("infcall", class_maintenance, > &riscv_debug_infcall, _("\ > Set riscv inferior call debugging."), _("\ > @@ -2982,9 +3001,9 @@ of the stack unwinding mechanism."), > Set debugger's use of compressed breakpoints."), _(" \ > Show debugger's use of compressed breakpoints."), _("\ > Debugging compressed code requires compressed breakpoints to be used. If= \n \ > -left to 'auto' then gdb will use them if $misa indicates the C extension= \n \ > -is supported. If that doesn't give the correct behavior, then this optio= n\n\ > -can be used."), > +left to 'auto' then gdb will use them if the existing instruction is a\n= \ > +compressed instruction. If that doesn't give the correct behavior, then\= n \ > +this option can be used."), > NULL, > show_use_compressed_breakpoints, > &setriscvcmdlist, >=20 > --=20 > John Baldwin >=20 > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0