From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103722 invoked by alias); 22 Sep 2015 16:36:28 -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 103713 invoked by uid 89); 22 Sep 2015 16:36:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f54.google.com Received: from mail-pa0-f54.google.com (HELO mail-pa0-f54.google.com) (209.85.220.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 22 Sep 2015 16:36:25 +0000 Received: by pacgz1 with SMTP id gz1so10965830pac.3 for ; Tue, 22 Sep 2015 09:36:23 -0700 (PDT) X-Received: by 10.66.190.135 with SMTP id gq7mr32368886pac.65.1442939783582; Tue, 22 Sep 2015 09:36:23 -0700 (PDT) Received: from E107787-LIN (power-aix.osuosl.org. [140.211.15.154]) by smtp.gmail.com with ESMTPSA id y5sm3171556pbt.77.2015.09.22.09.36.21 (version=TLS1_2 cipher=AES128-SHA256 bits=128/128); Tue, 22 Sep 2015 09:36:23 -0700 (PDT) From: Yao Qi To: James Bowman Cc: "gdb-patches\@sourceware.org" Subject: Re: [PATCH, FT32] Proper support for address <-> pointer conversions References: Date: Tue, 22 Sep 2015 16:36:00 -0000 In-Reply-To: (James Bowman's message of "Mon, 21 Sep 2015 23:06:48 +0000") Message-ID: <86wpvi9zst.fsf@gmail.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: 2015-09/txt/msg00531.txt.bz2 James Bowman writes: > FT32 is a Harvard architecture with two address spaces -- RAM and flash. > This patch properly implements the address to pointer conversion method, > and its reverse. There are some other small fixes to handle address > spaces. Hi James, Thanks for your patch. A few comments below... Please split your patch, in order to place each independent small fix in one patch. > > OK to apply? > > 2015-09-21 James Bowman > > * ft32-tdep.c: Properly support address <-> pointer conversions, > so that FT32's address spaces (RAM and flash) work correctly > when dereferencing pointers in gdb. > * ft32-tdep.h: Add a type for an address space 1 pointer. The changelog entry looks incorrect to me. Please read https://sourceware.org/gdb/wiki/ContributionChecklist > > > diff --git a/gdb/ft32-tdep.c b/gdb/ft32-tdep.c > index 2e5deca..d6e8c6d 100644 > --- a/gdb/ft32-tdep.c > +++ b/gdb/ft32-tdep.c > @@ -117,7 +117,7 @@ static struct type * > ft32_register_type (struct gdbarch *gdbarch, int reg_nr) > { > if (reg_nr =3D=3D FT32_PC_REGNUM) > - return builtin_type (gdbarch)->builtin_func_ptr; > + return gdbarch_tdep (gdbarch)->pc_type; > else if (reg_nr =3D=3D FT32_SP_REGNUM || reg_nr =3D=3D FT32_FP_REGNUM) > return builtin_type (gdbarch)->builtin_data_ptr; > else > @@ -270,6 +270,88 @@ ft32_skip_prologue (struct gdbarch *gdbarch, CORE_AD= DR pc) > return pc; > } >=20=20 > +/* Convert from address to pointer and vice-versa. */ > + > +static void > +ft32_address_to_pointer (struct gdbarch *gdbarch, > + struct type *type, gdb_byte *buf, CORE_ADDR addr) > +{ > + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > + > + store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order, > + addr); > +} ft32_address_to_pointer is exactly the same as unsigned_address_to_pointer, which is the default implementation of gdbarch method address_to_pointer. IOW, we don't need this function. > + > +static CORE_ADDR > +ft32_pointer_to_address (struct gdbarch *gdbarch, > + struct type *type, const gdb_byte *buf) We need comments to this function, like, "Implementation of ....." > +{ > + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > + CORE_ADDR addr > + =3D extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order); A blank line is needed here. > + if (TYPE_ADDRESS_CLASS_1 (type)) > + return addr; > + else > + return addr | RAM_BIAS; > +} > + > +static CORE_ADDR > +ft32_integer_to_address (struct gdbarch *gdbarch, > + struct type *type, const gdb_byte *buf) > +{ > + ULONGEST addr =3D unpack_long (type, buf); > + > + return addr; > +} > + Do we really need this function? Looks it makes no differences after this hook is installed to gdbarch. >=20=20 > static CORE_ADDR > @@ -498,6 +580,15 @@ ft32_gdbarch_init (struct gdbarch_info info, struct = gdbarch_list *arches) > tdep =3D XNEW (struct gdbarch_tdep); > gdbarch =3D gdbarch_alloc (&info, tdep); >=20=20 > + /* Create a type for PC. We can't use builtin types here, as they may= not > + be defined. */ > + tdep->void_type =3D arch_type (gdbarch, TYPE_CODE_VOID, 1, "void"); > + tdep->func_void_type =3D make_function_type (tdep->void_type, NULL); > + tdep->pc_type =3D arch_type (gdbarch, TYPE_CODE_PTR, 4, NULL); > + TYPE_TARGET_TYPE (tdep->pc_type) =3D tdep->func_void_type; > + TYPE_UNSIGNED (tdep->pc_type) =3D 1; > + TYPE_INSTANCE_FLAGS (tdep->pc_type) |=3D TYPE_INSTANCE_FLAG_ADDRESS_CL= ASS_1; > + > set_gdbarch_read_pc (gdbarch, ft32_read_pc); > set_gdbarch_write_pc (gdbarch, ft32_write_pc); > set_gdbarch_unwind_sp (gdbarch, ft32_unwind_sp); > @@ -510,6 +601,10 @@ ft32_gdbarch_init (struct gdbarch_info info, struct = gdbarch_list *arches) >=20=20 > set_gdbarch_return_value (gdbarch, ft32_return_value); >=20=20 > + set_gdbarch_address_to_pointer (gdbarch, ft32_address_to_pointer); > + set_gdbarch_pointer_to_address (gdbarch, ft32_pointer_to_address); > + set_gdbarch_integer_to_address (gdbarch, ft32_integer_to_address); > + > set_gdbarch_skip_prologue (gdbarch, ft32_skip_prologue); > set_gdbarch_inner_than (gdbarch, core_addr_lessthan); > set_gdbarch_breakpoint_from_pc (gdbarch, ft32_breakpoint_from_pc); > @@ -535,6 +630,12 @@ ft32_gdbarch_init (struct gdbarch_info info, struct = gdbarch_list *arches) > /* Support simple overlay manager. */ > set_gdbarch_overlay_update (gdbarch, simple_overlay_update); >=20=20 > + set_gdbarch_address_class_type_flags (gdbarch, ft32_address_class_type= _flags); > + set_gdbarch_address_class_name_to_type_flags > + (gdbarch, ft32_address_class_name_to_type_flags); > + set_gdbarch_address_class_type_flags_to_name > + (gdbarch, ft32_address_class_type_flags_to_name); > + > return gdbarch; > } >=20=20 > diff --git a/gdb/ft32-tdep.h b/gdb/ft32-tdep.h > index 5c52480..f236163 100644 > --- a/gdb/ft32-tdep.h > +++ b/gdb/ft32-tdep.h > @@ -22,7 +22,12 @@ >=20=20 > struct gdbarch_tdep > { > - /* gdbarch target dependent data here. Currently unused for FT32. */ > + /* Type for void. */ > + struct type *void_type; > + /* Type for a function returning void. */ > + struct type *func_void_type; > + /* Type for a pointer to a function. Used for the type of PC. */ > + struct type *pc_type; Only pc_type is used (in ft32_register_type), so we don't need to add fields void_type and func_void_type. --=20 Yao (=E9=BD=90=E5=B0=A7)