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

[Anton Kolesov <Anton dot Kolesov at synopsys dot com>]

Hi Pedro,

Thank you for your review.

> 
> > Since it is expected that 7.12 will be the last release that supports building
> > GDB with C compiler (https://sourceware.org/ml/gdb/2016-05/msg00016.html),
> > this patch contain some code that cannot be compiled with older GCC's default
> > -std=gnu89 for C, in particular variables are declared at the first use,
> > rather then at the top of the function.
> 
> 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.


> 
> In general the patch looks pretty good to me, but I have a few
> comments below.
> 
> >
> >  * 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.


> > +/* XML target description features.  */
> > +
> > +static const char *const core_v2_feature_name = "org.gnu.gdb.arc.core.v2";
> > +static const char *const
> > +  core_reduced_v2_feature_name = "org.gnu.gdb.arc.core-reduced.v2";
> > +static const char *const
> > +  core_arcompact_feature_name = "org.gnu.gdb.arc.core.arcompact";
> > +static const char *const
> > +  aux_minimal_feature_name = "org.gnu.gdb.arc.aux-minimal";
> > +
> 
> Do these need to be pointers instead of arrays?

Probably not, I will correct that.


> 
> 
> > +/* Push dummy call return code sequence.
> > +
> > +   We don't actually push any code.  We just identify where a breakpoint can
> > +   be inserted to which we are can return and the resume address where we
> > +   should be called.
> 
> Something seems odd with the last sentence.  "to which we are can"?

"to which we are to return", I think.


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


> > +
> > +/* 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.


> > +
> > +/* 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 values are
> > +   in the R0,
> 
> s/in the R0/in R0/ ?
> 
> >  while 8-byte values are in the R1.
> 
> ITYM "are in R0-R1".  The function seems to extract it out
> of both.

Yes.


> 
> > +
> > +   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. 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 debugger user
> > +      has issued a "return <value>" command to request immediate return from
> > +      the current function with the given result; or
> 
> Parens opened but never closed.
> 
> > +
> > +   2. Get the return value ((this is for the situation where the debugger
> > +      user has executed a "call <function>" command to execute the specified
> > +      function in the target program, and that function has a non-void result
> > +      which must be returned to the user.  */
> 
> Spurious double-parens, and never closed.
> 
> These generic comments would be better placed in gdbarch.sh, and then
> here just say:
> 
> /* Implement the XXX gdbarch method.  */
> 
> 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 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).


> > +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 = arc_delayed_print_insn (*pcptr, &di);
> 
> 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 = 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?


> > +
> > +  /* If we are asked to unwind the PC, then we need to return BLINK instead:
> > +     the saved value of PC points into this frame's function's prologue, not
> > +     the next frame's function's resume location.  `frame_unwind_got_constant`
> > +     is used to return non_lvalue, because it is not possible to modify PC
> 
> s/non_lvalue/not_lval
> 
> > +     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?


> > +
> > +struct gdbarch_tdep
> > +  {
> > +  };
> 
> Formatting is off.  { should on column 0.
> 
> Empty structs with no fields is not valid C89 either.
> 
> How about just simply dropping these placeholders, and add
> them back when they're actually needed?

Ok.


> 
> > +important to GDB are not "core" registers in ARC.  To simplify internal 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 register
> > +number less then 68.
> 
> 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?

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.

Anton

> 
> Thanks,
> Pedro Alves