From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130475 invoked by alias); 15 Sep 2016 11:39:55 -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 129660 invoked by uid 89); 15 Sep 2016 11:39:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=designers, puzzled, intercept, dates X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Sep 2016 11:39:52 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D37A76646; Thu, 15 Sep 2016 11:39:50 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8FBdnOA027356; Thu, 15 Sep 2016 07:39:49 -0400 Subject: Re: [PATCH 1/2] arc: New Synopsys ARC port To: Anton Kolesov , "gdb-patches@sourceware.org" References: <1472633399-32477-1-git-send-email-Anton.Kolesov@synopsys.com> <806a2777-4ee5-8512-cf3b-d1ac590d72b5@redhat.com> <39A54937CC95F24AA2F794E2D2B66B13581A07C7@DE02WEMBXB.internal.synopsys.com> Cc: "Francois.Bedard@synopsys.com" From: Pedro Alves Message-ID: <2cdb1ba5-ab28-d913-0c55-aa67c88b6e26@redhat.com> Date: Thu, 15 Sep 2016 11:39:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <39A54937CC95F24AA2F794E2D2B66B13581A07C7@DE02WEMBXB.internal.synopsys.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-09/txt/msg00137.txt.bz2 Hi Anton, On 09/01/2016 08:54 PM, Anton Kolesov wrote: >>> * 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. >> >> 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. OK, I was just curious. I think the best is to preserve the dates you already have. > >> >>> + >>> + ARC does not necessarily have an executable stack, so we can't put the >>> + return breakpoint there. >> >> 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. I see. That's unfortunate. I think that sooner or later you'll want to make the openocd port intercept exceptions. But what you have will do. > > >>> + >>> +/* 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. RESERVED and >>> + LIMM are non-existent registers. >> >> 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 space. > 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 designers. > 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 later 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 see if I > can remove them from internal GDB list, while maintaining compatibility. Yes, you should not need those in the target descriptions. If you need to support old stubs that did _not_ use XML target descriptions, and included space for those registers in in the g/G packet, then what you need to do is add a fallback XML target description built into gdb that matches the layout of the g/G packet of those stubs. That's what we do on ARM too -- older stubs used to always include space for the FPA registers, even if they didn't have them. See gdb/features/arm-fpu.xml and gdb/features/arm-with-m-fpa-layout.xml for example. And then we register a "g/G packet size" -> "built-in xml target description" guess. See arm_register_g_packet_guesses: /* For backward-compatibility we allow two 'g' packet lengths with the remote protocol depending on whether FPA registers are supplied. M-profile targets do not have FPA registers, but some stubs already exist in the wild which use a 'g' packet which supplies them albeit with dummy values. The packet format which includes FPA registers should be considered deprecated for M-profile targets. */ static void arm_register_g_packet_guesses (struct gdbarch *gdbarch) { > >> >>> + >>> + TODO: This implementation ignores the case of "complex double", where >>> + according to ABI, value is returned in the R0-R3 registers. >>> + >>> + TYPE is a returned value's type. VALBUF is a buffer for the returned >>> + value. */ >>> + >>> +static void >>> +arc_extract_return_value (struct gdbarch *gdbarch, struct type *type, >>> + struct regcache *regcache, gdb_byte *valbuf) >>> +{ >>> + unsigned int len = TYPE_LENGTH (type); >>> + >>> + if (arc_debug) >>> + debug_printf ("arc: extract_return_value\n"); >>> + >>> + if (len <= BYTES_IN_REGISTER) >> >> Where is BYTES_IN_REGISTER defined? > > arc-tdep.h. Ah. Somehow that eluded me. > Should I rename it to something like ARC_BYTES_IN_REGISTER or > ARC_REGISTER_SIZE? Yes please. > >>> +/* 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 doesn'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 = arc_get_disassembler (exec_bfd); >> >> Seems like this will crash if there's no exec_bfd ? > > Indeed. I forgot to send my patch to binutils, which will make it gracefully handle > the case of (exec_bfd == NULL). Great, I see that that's now in. BTW, I see that arc_extension_map is a global regenerated whenever the binary changes. At some point you'll want to make it not be a global, since gdb supports debugging more than one process at the same time (with extended-remote). Another think you may consider is that the XML target description supports an element, which is really the bfd target name. You could use that to compute the right disassembler when you don't have a bfd. > >>> + /* No line info so use current PC. */ >>> + prologue_end = prev_pc; >>> + else if (sal.end < prologue_end) >>> + /* The next line begins after the function end. */ >>> + prologue_end = sal.end; >>> + >>> + prologue_end = min (prologue_end, prev_pc); >>> + } >> >> Indentation look odd here. > > if-else is at level 3, so indented by 6 spaces. Body is at level 4, so indented by > 1 tab instead of 8 spaces. Which is far as I understood is the proper way for 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 shift, > since tabs align to column numbers %8, instead of having a fixed width instead of > having fixed width. So in my response email indentation looks even weirder. Is > there anything I should do differently? No. I'm used to considering that tab/spaces + ">" interaction when reviewing, but this time I failed to spot it. My bad. >>> + directly (one can modify BLINK, if they wish). If >>> + trad_frame_get_prev_register (ARC_BLINK_REGNUM) would be used here, 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 technically, >>> + however sematically could be confusing, so better to avoid this, like >>> + other arches do. */ >> >> 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 modified value of > LR register, while in AArch64 and ARC it is just a register value as-is. Old ARC code, > which used to be based on GDB 6.8 contained some custom logic of its own, that 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? If there's no good reason, I'd start by using standard trad_frame_get_prev_register. There may be a reason, and if we do find one, we can then add a test and comment. As for aarch64, no idea. >> 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 at 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 pass to > set_gdbarch_pc_regnum (), and "regnum" attribute in XML can be anything else or > not set at all - that would be a number used by GDB's p/P packet and it will be > unrelated to internal numbers. Is this correct? Correct. If you haven't found it yet, the "maint print remote-registers" command is handy to see the number mapping that gdb ended up with at run time, as well as the register offsets in the g/G packet. > > I think, it could be that I was confused because gdbarch functions end with > "_regnum()", so I thought that regnums in the XML must be identical to regnums > I pass to gdbarch. Yeah, they don't. Thanks, Pedro Alves