From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2439 invoked by alias); 1 Sep 2016 19:54:24 -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 2429 invoked by uid 89); 1 Sep 2016 19:54:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=designers, tabs, OpenOCD, openocd X-HELO: smtprelay.synopsys.com Received: from smtprelay.synopsys.com (HELO smtprelay.synopsys.com) (198.182.47.9) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Sep 2016 19:54:13 +0000 Received: from us02secmta2.synopsys.com (us02secmta2.synopsys.com [10.12.235.98]) by smtprelay.synopsys.com (Postfix) with ESMTP id D60AF24E0E82; Thu, 1 Sep 2016 12:54:11 -0700 (PDT) Received: from us02secmta2.internal.synopsys.com (us02secmta2.internal.synopsys.com [127.0.0.1]) by us02secmta2.internal.synopsys.com (Service) with ESMTP id A40CA55F13; Thu, 1 Sep 2016 12:54:11 -0700 (PDT) Received: from mailhost.synopsys.com (mailhost1.synopsys.com [10.12.238.239]) by us02secmta2.internal.synopsys.com (Service) with ESMTP id 6EE0B55F02; Thu, 1 Sep 2016 12:54:11 -0700 (PDT) Received: from mailhost.synopsys.com (localhost [127.0.0.1]) by mailhost.synopsys.com (Postfix) with ESMTP id 5725A3D2; Thu, 1 Sep 2016 12:54:11 -0700 (PDT) Received: from us01wehtc1.internal.synopsys.com (us01wehtc1.internal.synopsys.com [10.12.239.235]) by mailhost.synopsys.com (Postfix) with ESMTP id 33D273CB; Thu, 1 Sep 2016 12:54:11 -0700 (PDT) Received: from DE02WEHTCA.internal.synopsys.com (10.225.19.92) by us01wehtc1.internal.synopsys.com (10.12.239.235) with Microsoft SMTP Server (TLS) id 14.3.266.1; Thu, 1 Sep 2016 12:54:10 -0700 Received: from DE02WEMBXB.internal.synopsys.com ([fe80::95ce:118a:8321:a099]) by DE02WEHTCA.internal.synopsys.com ([::1]) with mapi id 14.03.0266.001; Thu, 1 Sep 2016 21:54:08 +0200 From: Anton Kolesov To: Pedro Alves , Anton Kolesov , "gdb-patches@sourceware.org" CC: "Francois.Bedard@synopsys.com" Subject: RE: [PATCH 1/2] arc: New Synopsys ARC port Date: Thu, 01 Sep 2016 19:54:00 -0000 Message-ID: <39A54937CC95F24AA2F794E2D2B66B13581A07C7@DE02WEMBXB.internal.synopsys.com> References: <1472633399-32477-1-git-send-email-Anton.Kolesov@synopsys.com> <806a2777-4ee5-8512-cf3b-d1ac590d72b5@redhat.com> In-Reply-To: <806a2777-4ee5-8512-cf3b-d1ac590d72b5@redhat.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-SW-Source: 2016-09/txt/msg00028.txt.bz2 Hi Pedro, Thank you for your review. >=20 > > Since it is expected that 7.12 will be the last release that supports b= uilding > > GDB with C compiler (https://sourceware.org/ml/gdb/2016-05/msg00016.htm= l), > > this patch contain some code that cannot be compiled with older GCC's d= efault > > -std=3Dgnu89 for C, in particular variables are declared at the first u= se, > > rather then at the top of the function. >=20 > This has not happened yet, and we still support building with a > C compiler. So this can't go in as is until that is resolved... Will be fixed. >=20 > In general the patch looks pretty good to me, but I have a few > comments below. >=20 > > > > * GDB and GDBserver now build with a C++ compiler by default. > > diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c > > new file mode 100644 > > index 0000000..6e0e68d > > --- /dev/null > > +++ b/gdb/arc-tdep.c > > @@ -0,0 +1,1371 @@ > > +/* Target dependent code for ARC arhitecture, for GDB. > > + > > + Copyright 2005-2016 Free Software Foundation, Inc. >=20 > Does this file really contain code that is that old? This date was there when I've inherited the project, although I don't know if there is any code remaining from the initial port - there was lots of work on GDB but not complete rewrites. Linux was initially ported to ARC somewhere in 2006 and it should be that GDB was ported at the same timeframe. Nobody from those times is in current team, so hard to know for sure. And proper commit history of internal project development has been lost during multiple source control migrations since that time, so it is hard to figure out exact dates. > > +/* XML target description features. */ > > + > > +static const char *const core_v2_feature_name =3D "org.gnu.gdb.arc.cor= e.v2"; > > +static const char *const > > + core_reduced_v2_feature_name =3D "org.gnu.gdb.arc.core-reduced.v2"; > > +static const char *const > > + core_arcompact_feature_name =3D "org.gnu.gdb.arc.core.arcompact"; > > +static const char *const > > + aux_minimal_feature_name =3D "org.gnu.gdb.arc.aux-minimal"; > > + >=20 > Do these need to be pointers instead of arrays? Probably not, I will correct that. >=20 >=20 > > +/* Push dummy call return code sequence. > > + > > + We don't actually push any code. We just identify where a breakpoi= nt can > > + be inserted to which we are can return and the resume address where= we > > + should be called. >=20 > Something seems odd with the last sentence. "to which we are can"? "to which we are to return", I think. >=20 > > + > > + ARC does not necessarily have an executable stack, so we can't put = the > > + return breakpoint there. >=20 > What actually goes wrong if you put the breakpoint there? GDB should > manage to figure out a SIGSEGV at that address means the function > returned. x86-64 GNU/Linux has no executable stack either, for instance, > and: SIGSEGV will happen only if we have some OS or another setup to check for it, like on Linux targets. With simple baremetal applications on error we just get a CPU exception, which is handled by the application itself or some sort of an RTOS if one is involved. Our OpenOCD port doesn't know any of those specifics, so it doesn't intercept the memory error and doesn't report SIGSEGV to the GDB client. > > + > > +/* Determine whether a register can be read. > > + > > + All of the registers in the required set are readable. There are > > + write-only registers among the non-required, but since GDB doesn't = know > > + anything about them, access is controlled by the GDB server. RESER= VED and > > + LIMM are non-existent registers. >=20 > What does this mean, "non-existent registers" ? If they don't > exist, then why do they have names and are handled here? > I'm puzzled on what's the point of a register that can't be > neither read nor written? It's not physical registers but addresses in the core register address spac= e. Address-field in the instruction encoding is 6-bit long, so there can be up= to 64 registers. However, in the real CPU out of those: 1) R0-R31 are real core registers (though, some of them are optional) 2) R32-R59 are extension registers, which can be added by the CPU designer= s. For example, FPU or vector registers, which will be treated as a general purpose registers. 3) R60 is a loop counter for hardware loops 4) R61 is the "reserved" address, so that we can add a new register to base ISA without breaking compatibility with hardware extensions already developed by our customers. Hence there is no register per se, but there is an address for it. Currently CPU will just have an exception if instruction tries to use that register. 5) R62 is a "long immediate data indicator". If instruction uses this as an argument, then this means that arguments is not in the register, but in the following 4-bytes of instruction stream. 6) R63 is a "PCL", which is a read-only 4-byte aligned instruction pointer. Unfortunately, some of the older gdbserver implementations considered LIMM = and Reserved as a "real" registers - they had their own space in the g/G packet= , so when I was adding support for XML target descriptions, I've left those two = as optional regs in the "core registers" feature, so that GDB would work fine = with those old gdbservers. Current GDB-servers for ARC, that generate XML target descriptions on their own (OpenOCD for ARC, our proprietary instruction simulator), never include those registers in the description, since they are not real. While looking into the target descriptions for your feedback late= r in the email, I've started to realize that it might be that I don't need those= hacked "registers" to support old remote servers. For second patch version I'll se= e if I can remove them from internal GDB list, while maintaining compatibility. > > + > > +/* Get the return value of a function from the registers/memory used to > > + return it, according to the convention used by the ABI - 4-bytes va= lues are > > + in the R0, >=20 > s/in the R0/in R0/ ? >=20 > > while 8-byte values are in the R1. >=20 > ITYM "are in R0-R1". The function seems to extract it out > of both. Yes. >=20 > > + > > + TODO: This implementation ignores the case of "complex double", whe= re > > + according to ABI, value is returned in the R0-R3 registers. > > + > > + TYPE is a returned value's type. VALBUF is a buffer for the return= ed > > + value. */ > > + > > +static void > > +arc_extract_return_value (struct gdbarch *gdbarch, struct type *type, > > + struct regcache *regcache, gdb_byte *valbuf) > > +{ > > + unsigned int len =3D TYPE_LENGTH (type); > > + > > + if (arc_debug) > > + debug_printf ("arc: extract_return_value\n"); > > + > > + if (len <=3D BYTES_IN_REGISTER) >=20 > Where is BYTES_IN_REGISTER defined? arc-tdep.h. Should I rename it to something like ARC_BYTES_IN_REGISTER or ARC_REGISTER_SIZE? > > + > > +/* Determine how a result of particular type is returned. > > + > > + Return the convention used by the ABI for returning a result of the= given > > + type from a function; it may also be required to: > > + > > + 1. Set the return value (this is for the situation where the debugg= er user > > + has issued a "return " command to request immediate retur= n from > > + the current function with the given result; or >=20 > Parens opened but never closed. >=20 > > + > > + 2. Get the return value ((this is for the situation where the debug= ger > > + user has executed a "call " command to execute the spe= cified > > + function in the target program, and that function has a non-void= result > > + which must be returned to the user. */ >=20 > Spurious double-parens, and never closed. >=20 > These generic comments would be better placed in gdbarch.sh, and then > here just say: >=20 > /* Implement the XXX gdbarch method. */ >=20 > Augmented with mention of any ARC specifics if any worth mentioning. Ok. > > +/* arc_get_disassembler () may return different functions depending on= bfd > > + type, so it is not possible to pass print_insn directly to > > + set_gdbarch_print_insn (). Instead this wrapper function is used. = It also > > + may be used by other functions to get disassemble_info for address.= It is > > + important to note, that those print_insn from opcodes always print > > + instruction to the stream specified in the info, so if that is not = desired, > > + then print_insn in the info should be set to some function that doe= sn't > > + print anything, like arc_scream_into_the_void from this file. */ > > + > > +static int > > +arc_delayed_print_insn (bfd_vma addr, struct disassemble_info *info) > > +{ > > + int (*print_insn) (bfd_vma, struct disassemble_info *); > > + print_insn =3D arc_get_disassembler (exec_bfd); >=20 > Seems like this will crash if there's no exec_bfd ? Indeed. I forgot to send my patch to binutils, which will make it gracefull= y handle the case of (exec_bfd =3D=3D NULL). > > +static const unsigned char * > > +arc_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, > > + int *lenptr) > > +{ > > + struct disassemble_info di; > > + arc_initialize_disassembler (gdbarch, &di); > > + size_t length_with_limm =3D arc_delayed_print_insn (*pcptr, &di); >=20 > Seems to me that if you need to do this instead of being able > to use gdb_insn_length, then that leaves gdb_insn_length calls in > common code broken. So, doesn't gdb_insn_length work here? Why not? I didn't know about gdb_insn_length. It should work here. > > + /* No line info so use current PC. */ > > + prologue_end =3D prev_pc; > > + else if (sal.end < prologue_end) > > + /* The next line begins after the function end. */ > > + prologue_end =3D sal.end; > > + > > + prologue_end =3D min (prologue_end, prev_pc); > > + } >=20 > Indentation look odd here. if-else is at level 3, so indented by 6 spaces. Body is at level 4, so inde= nted by 1 tab instead of 8 spaces. Which is far as I understood is the proper way f= or GNU projects, for example target.c:target_section_by_addr. So when "> " or "+" = are inserted in front in the patch file, if-else shifts, but the body doesn't s= hift, since tabs align to column numbers %8, instead of having a fixed width inst= ead of having fixed width. So in my response email indentation looks even weirder.= Is there anything I should do differently? > > + > > + /* If we are asked to unwind the PC, then we need to return BLINK in= stead: > > + the saved value of PC points into this frame's function's prologu= e, not > > + the next frame's function's resume location. `frame_unwind_got_c= onstant` > > + is used to return non_lvalue, because it is not possible to modif= y PC >=20 > s/non_lvalue/not_lval >=20 > > + directly (one can modify BLINK, if they wish). If > > + trad_frame_get_prev_register (ARC_BLINK_REGNUM) would be used her= e, than > > + it would be possible to modify PC of frame via altering BLINK of > > + next_frame, or at least I believe that should be possible technic= ally, > > + however sematically could be confusing, so better to avoid this, = like > > + other arches do. */ >=20 > What other archs do this for this purpose? Hm. I probably looked at aarch64 when doing that, but it seems that aarch64= doesn't really have a good reason to do that as well - the code was copy-pasted and= modified from arm-tdep, where it was needed, because PC value was actually a modifie= d value of LR register, while in AArch64 and ARC it is just a register value as-is. Ol= d ARC code, which used to be based on GDB 6.8 contained some custom logic of its own, t= hat was making PC value a not_lval, which I've replaced with standard got_constant = here. So I should just replace REGNUM with BLINK and use standard trad_frame_get_prev_register, without making it "constant"? Should aarch64 = be fixed as well, or there is some difference I'm missing? > > + > > +struct gdbarch_tdep > > + { > > + }; >=20 > Formatting is off. { should on column 0. >=20 > Empty structs with no fields is not valid C89 either. >=20 > How about just simply dropping these placeholders, and add > them back when they're actually needed? Ok. >=20 > > +important to GDB are not "core" registers in ARC. To simplify interna= l GDB > > +design some requirements are applied to ARC target descriptions: > > + > > +@itemize @bullet > > +@item > > +One of the core registers features must be present. At all occasions = GDB > > +register number must be equal to architectural number of that register= , so for > > +example regnum of @samp{blink} is always 31. > > + > > +@item @samp{org.gnu.gdb.arc.aux-minimal} is mandatory and registers in= it has > > +fixed GDB register numbers. > > + > > +@item All other features are optional and cannot have registers with r= egister > > +number less then 68. >=20 > GDB should be mapping the tdesc numbers to internal numbers, so I don't > understand why do the register numbers matter in the target description a= t all? > It should only matter for the fallback descriptions built in gdb, in > order to match the layout of the g/G packets of remote servers that > don't send in tdescs over the wire, I believe? I see now, that I misunderstood that internal numbers are not related to the regnums in the XML. So, when invoking the tdesc_numbered_register (), REGNO should be the internal register number, which must correspond to what I pas= s to set_gdbarch_pc_regnum (), and "regnum" attribute in XML can be anything els= e or not set at all - that would be a number used by GDB's p/P packet and it wil= l be unrelated to internal numbers. Is this correct? I think, it could be that I was confused because gdbarch functions end with= =20 "_regnum()", so I thought that regnums in the XML must be identical to regn= ums I pass to gdbarch. Anton >=20 > Thanks, > Pedro Alves