This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH 1/2] arc: New Synopsys ARC port
- From: Pedro Alves <palves at redhat dot com>
- To: Anton Kolesov <Anton dot Kolesov at synopsys dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Cc: "Francois dot Bedard at synopsys dot com" <Francois dot Bedard at synopsys dot com>
- Date: Thu, 15 Sep 2016 12:39:48 +0100
- Subject: Re: [PATCH 1/2] arc: New Synopsys ARC port
- Authentication-results: sourceware.org; auth=none
- References: <1472633399-32477-1-git-send-email-Anton.Kolesov@synopsys.com> <email@example.com> <39A54937CC95F24AA2F794E2D2B66B13581A07C7@DE02WEMBXB.internal.synopsys.com>
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,
> 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.
/* 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. */
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?
Ah. Somehow that eluded me.
> Should I rename it to something like ARC_BYTES_IN_REGISTER or
>>> +/* 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 <architecture> 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.