This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] arc: New Synopsys ARC port


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 <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.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]