Re: [PATCH] Add support for the Lattice Mico32 (LM32) architecture

[Joel Brobecker <brobecker at adacore dot com>]

Jon,

Sorry for the delay in reviewing this patch. This is a fairly big one,
and it's not often that one has a big enough window to fit in this
review :). On a side note, could you use "unified" diffs instead of
using "context" diffs. Most of us here find the latter much harder
to review (thanks!).

First off, I'll say that I can't review the changes to the simulator.
But I think we have a few fairly active maintainers for the simulator.
I'd just resend this part as a separate patch with an explicit subject
that says this is a simulator patch (with maybe a PING, this tends to
wake people up).

> gdb/
> 2008-12-15  Jon Beniston <jon@beniston.com>
>         
>         * MAINTAINERS: Add Jon Beniston as lm32 maintainer.        

I cannot approve this part yet, as I haven't had the change before
this patch to review your code and make sure that you're able to
follow the GDB conventions in your code.  As you'll see, there
are a few things that you didn't follow, so I think that it will
be beneficial for you if we continue reviewing your patches before
you commit. Given the nature of the issues I found, most are easy
to learn and we should be able to nominate you soon.

Also, I'll say it now: I had a lot of comments, and maybe it'll feel
bad, but it should not feel that way. I don't think you're very far
away from having something I can approve. A lot of it looks good to
me in principle, and you're been commented on issues that should be
easy to fix.

> gdb/testsuite
> 2008-12-15  Jon Beniston <jon@beniston.com>
> 
>         * gdb.asm/asm-source.exp: Add lm32 target.

This part is fine.

> include/gdb/
> 2008-12-15  Jon Beniston <jon@beniston.com>
> 
>         * sim-lm32.h: New file.

Please make sure to ask the sim maintainer to approve this part...

>   	ia64		--target=ia64-linux-gnu ,-Werror
>   			(--target=ia64-elf broken)
>   
> +         lm32            --target=lm32-elf ,-Werror
> +                         Jon Beniston, jon@beniston.com

As much as it pains me to say this (I **HATE** tabs, I absolutely
abhore tabs, I cannot believe that we're still using them. Aaaahhh,
I feel better now): You need to use tabs in this case to be consistent
with the rest of this section.  Tabs before "lm32", and tabs between
"lm32" and the --target.

> --- 1299,1306 ----
>   	irix5-nat.c \
>   	libunwind-frame.c \
>   	linux-fork.c \
> ! 	lm32-linux-tdep.c lm32-tdep.c \
> !         m68hc11-tdep.c \
>   	m32r-tdep.c \
>   	m32r-linux-nat.c m32r-linux-tdep.c \
>   	m68k-tdep.c \

You should replace the spaces on the second line by a tab.

> + lm32*-*-uclinux*)		

This is really nit-picking, but I looked at other GNU tools, and
it looks like their configure scripts only recognize "lm32". I think
we should be consistent and remove the first "*".

Also, I'm wondering about only accepting "uclinux", as opposed to
"*linux*". BFD, for instance, does not treat linux and uclinux
differently. Unless you think that GDB will have to treat the two
systems differently, perhaps it would make sense to follow BFD's
example.

> +         gdb_target_obs="lm32-tdep.o lm32-linux-tdep.o" 

Tab instead of spaces...

> +         gdb_target_obs="lm32-tdep.o" 

Same here...

I started reviewing this patch while reading it, and after reviewing
the first file, I finally realized that this first file on the list
is called lm32-linux-tdep.c.  Purely from the contents of that file,
I was convinced that I was reviewing lm32-tdep.c.  I don't think
there is anything in that file that is specific to linux. Is there?

> +    Copyright (C) 2008 Free Software Foundation, Inc.

It's already 2009, so you'll need to add 2009 to the copyright date.

> + #include <signal.h>

Can you explain what this is for? Intuitively, I don't think this
can be correct since you are including the signal.h from the host,
which might be different from the one on the target...

> + #include <string.h>

For portability reasons, GDB always includes "gdb_string.h".

> + int lm32_software_single_step (struct frame_info *frame);

I can't find the body for this function. Looks like your patch
is incomplete???  I'm also a little bit surprised that this is
unilaterally needed. Doesn't uclinux provide single-stepping,
for instance? How about the simulator?

> + enum {
> +   NORMAL_SIGTRAMP = 1,
> +   /* RT_SIGTRAMP = 2 */
> + };

As far as I can tell, this isn't really useful. I can only see
this enum being used in one place, which is lm32_linux_pc_in_sigtramp,
which is unused. It looks like either this is a left over that is
no longer needed, or, as I suspect, there is a piece missing in your
patch.

> + #define LM32_ELF_NGREG 32
> + typedef unsigned char lm32_elf_greg_t[4];

I think that you might want to use gdb_byte instead of "unsigned char".

> + #define LM32_UIMM16(insn)       (insn & 0xffff)
> + #define LM32_IMM26(insn)        ((((long)insn & 0x3ffffff) << 6) >> 6)

These two macros seem to be unused at the moment. It's OK to leave them
if you think they might become useful at one point. Otherwise, let's
delete them.

> + struct lm32_unwind_cache

To be consistent with GDB's usual naming conventions, would you mind
renaming this structure to lm32_frame_cache, please?

> + {
> +   /* The previous frame's inner most stack address.  Used as this
> +      frame ID's stack_addr.  */
> +   CORE_ADDR prev_sp;

This looked suspicious to me, and indeed, it looks like it's unused
(apart from being computed).  Contrary to what your comment implies,
we usually use the cache->base as the frame's stack_addr... (which is
why I found this field a little strange)

> +   /* Size of frame */
> +   int size;

Similarly, it looks like this information is only needed to compute
the frame prev_sp. If this is the case, and assuming that prev_sp
can go too, then this can been jettisoned as well.

> +   /* Whether the function uses fp as a frame pointer */
> +   int uses_fp;

Same here. This field is set, but doesn't seem to be ever used.

> + static int
> + lm32_linux_pc_in_sigtramp (CORE_ADDR pc, char *name)

As said above, I suspect that you are missing a piece of code that
uses this function.  Otherwise, this function is unused and should
be deleted.

> + static void 
> + lm32_linux_supply_gregset (const struct regset *regset,
> +                           struct regcache *regcache,
> + 			  int regnum, const void *gregs, size_t len)

Tabs issue here again...

> + 	fprintf(stderr, "%s:%d\n", __FILE__, __LINE__);

I suspect that this trace wasn't supposed to be in this patch.
If you find such traces occasionally useful, you might want to consider
introducing traces conditional on a "debug" setting.  See how we use
"debug_infrun" inside infrun.c for instance.

> +   for (regi = 1; regi <= 32; regi++)
> +     {
> + 	regcache_raw_supply (regcache, regi, &gregsetp->reg[regi]);
> +     }

A minor style issue: The curly braces are not used when there is
only one statement in the body of the loop.

> + static void
> + lm32_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> + {
> +   /* Set the sigtramp frame sniffer.  */
> +   //frame_unwind_append_sniffer (gdbarch, lm32_linux_sigtramp_frame_sniffer); 

This commented-out code should be removed.

> +   set_gdbarch_regset_from_core_section (gdbarch,
> +                                         lm32_linux_regset_from_core_section);

Tabs vs space... (sorry about that, it annoys me at least as much
as it must be annoying you right now)

> + static enum gdb_osabi
> + lm32_linux_elf_osabi_sniffer (bfd *abfd)
> + {
> +   int elf_flags;
> + 
> +   elf_flags = elf_elfheader (abfd)->e_flags;
> + 
> +   if (elf_flags & EF_LM32_MACH)
> +     return GDB_OSABI_LINUX;
> + 	else

The "else" is incorrectly indented.

> + static int 
> + lm32_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> +                              struct reggroup *group)

Indentation issue...

> +   if (group == general_reggroup) 
> +     {

Can you remove the unnecessary curly braces, please?

> +       return    ((regnum >= LM32_R0_REGNUM) && (regnum <= LM32_RA_REGNUM))
> +              || (regnum == LM32_PC_REGNUM);

The formatting is strange, and will be destroyed if we run gdb_indent.sh.
The GNU Coding Standards (GCS) recommend that this be formatted as follow:

  return ((regnum >= LM32_R0_REGNUM && regnum <= LM32_RA_REGNUM)
          || regnum == LM32_PC_REGNUM);

(watchout for tabs, as you'll probably need them for the second line).

> +   else if (group == system_reggroup) 
> +     {
> +       return (   (regnum >= LM32_EA_REGNUM)
> +               && (regnum <= LM32_BA_REGNUM)
> +              );
> +     } 

Same here:

  else if (group == system_reggroup)
    return (regnum >= LM32_EA_REGNUM && regnum <= LM32_BA_REGNUM);

> + /* Return a name that corresponds to the given register number */

Can you add a '.' at the end of your sentence, followed by two spaces,
please? I realize this can be annoying to fix these, but we try to be
consistent and follow the GCS.

> +   if ((reg_nr < 0) || (reg_nr >= sizeof (register_names) / sizeof (register_names[0])))
> +     return NULL;

Formatting: The condition needs to be split in two lines to fit in
80 characters. Or better yet, use the ARRAY_SIZE macro defined for us
in libiberty.h (already included by defs.h).

> + /* Parse a functions prologue */

Typo: No 's' at the end of "function". Also, a period needs to be
added at the end of the sentence. However, when I looked at the
implementation, the function turned out to be completely different
from what I expected!

Traditionally, we call this function "..._analyze_prologue" and this
function usually performs two things:
  - analyze the prologue instructions to build a frame_cache object;
  - return the address where the last prologue instruction was found.

> + static CORE_ADDR
> + lm32_parse_prologue (CORE_ADDR pc)

With this in mind, it's a shame that you have two separate functions
for parsing and analyzing... You can have a look at i386-tdep.c to
see how it uses the same function to perform both tasks.

I'm not going to review the body of this function assuming that
you'll be able to merge it with the function that analyzes the function,
which you can extract from your "lm32_frame_unwind_cache" function,
I think.

> +   /* If we have line debugging information, then the end of the 
> +      prologue should the first assembly instruction of the first source line */
> +   if (find_pc_partial_function (pc, NULL, &func_addr, &func_end))
> +     {
> +       sal = find_pc_line (func_addr, 0);
> +       if (sal.end && sal.end < func_end) 
> +         return sal.end;        
> +     }

This part of the code can be replaced by a call to
skip_prologue_using_sal.

> + /* Create a breakpoint instruction */

Period and two spaces at the end of the sentence...

> + static const unsigned char *
> + lm32_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)
> + {
> +   static const unsigned char breakpoint[4] = {OP_RAISE << 2, 0, 0, 2};

"unsigned char" should be replaced by gdb_byte (same for the function
return type).

> + /* Setup registers and stack for faking a call to a function in the inferior */

period and spaces at the end of the sentence...

> +   /* Set the return address */
> +   /* If we're returning a large struct, a pointer to the address to
> +      store it at is passed as a first hidden parameter */
> +   /* Setup parameters */
> +       /* Promote small integer types to int */
> +       /* FIXME: Handle structures */  
> +   /* Update stack pointer */
> + /* Extract return value after calling a function in the inferior */
> +       /* Return value is returned in a single register */

Ditto.


> +     	  if (TYPE_LENGTH (arg_type) < 4)
> +     	    {
> + 	          arg_type = builtin_type_int32;
> + 	          arg = value_cast (arg_type, arg);

The last two lines are incorrectly formatted. You're also mixing
spaces and tabs, but I'm just about ready to give up on this one :-(.


> +       contents = (char *) value_contents (arg);        

I think that contents needs to be declared as a gdb_byte *, and that
this will allow the cast to go away.

> +       /* First num_arg_regs parameters go in registers, rest go on stack */

I should say that I really appreciate it when people make an effort
to comment their code as you did. I do personally follow a strict
rule when it comes to comments, as I try to write them in real English
as opposed to abbreviated English. The reason why I do this is because
I find that abbreviated English often makes it harder to read a comment,
thus reducing its effectiveness.

I was ready to let the comment above go, except that I noticed that the
second "go" should be "goes" and that you're missing the period at the
end of your sentence... I suggest:

  /* The First num_arg_regs parameters are passed by registers,
     and the rest as passed on the stack.  */

> +       if (i < num_arg_regs)    
> +         {
> +           regcache_cooked_write_unsigned (regcache, first_arg_reg + i, val);
> +         }

Can you remove the unnecessary curly braces, please?

> + static void
> + lm32_extract_return_value (struct type *type, struct regcache *regcache, 
> +                            gdb_byte *valbuf)

Tabs instead of spaces...


> +   if (   TYPE_CODE(type) != TYPE_CODE_STRUCT
> +       && TYPE_CODE(type) != TYPE_CODE_UNION
> +       && TYPE_CODE(type) != TYPE_CODE_ARRAY
> +       && TYPE_LENGTH (type) <= 4
> +      )

Formatting... I think this is the last time I'm going to mention
this. I'll let you fix the formatting everywhere else.

> +       /* 64-bit values are returned in a register pair */

Same for the missing periods at the end of your sentences. Can you
fix them throughout your patch, please?

> +       /* Aggregate types greater than a single register are returned in memory: 

I think that a period instead of a colon is better in this case.
But that's just a suggestion.

> + static void
> + lm32_store_return_value (struct type *type, struct regcache *regcache,
> + 			const gdb_byte *valbuf)

You're missing one space at the beginning of the second line.
Otherwise, the argument is not aligned...

> +       val = extract_unsigned_integer ((char *)valbuf + 4, len - 4);

I think the cast should be removed.

> + static enum return_value_convention
> + lm32_return_value (struct gdbarch *gdbarch, struct type *func_type,
> + 		    struct type *valtype, struct regcache *regcache, 

One space too many...

> +                     gdb_byte *readbuf, const gdb_byte *writebuf)

Tabs vs spaces...

> +   if ((code == TYPE_CODE_STRUCT
> +        || code == TYPE_CODE_UNION
> +        || code == TYPE_CODE_ARRAY
> +        || TYPE_LENGTH (valtype) > 8))

Can you remove the extra parens?

> + static struct frame_id
> + lm32_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
> + {
> +   CORE_ADDR sp = lm32_unwind_sp (gdbarch, this_frame);

I don't think this is correct, but I often get confused. In this case,
I think you're using the SP of the caller of THIS_FRAME.  I think you
want to use the SP of this frame, in which case you need to call
get_frame_register_unsigned.

> + struct lm32_unwind_cache *
> + lm32_frame_unwind_cache (struct frame_info *this_frame, 
> +                          void **this_prologue_cache)

I think that this function should be made static.
Also as discussed earlier, you should extract out the prologue
analyzing portion in order to factorize a little bit your code.

One last thing, just to make it consistent across GDB, can you
drop the "unwind" in the name of the function?

> +   /* The FUNC is easy.  */
> +   func = get_frame_func (this_frame);

Usually, the frame_func address is saved in the frame cache to avoid
having to recompute again. You've already needed to compute it while
computing the frame cache, so might as well avoid the recomputing...
For instance, the i386 frame code stores this address in a field
called "pc".

> +   /* Hopefully the prologue analysis either correctly determined the
> +      frame's base (which is the SP from the previous frame), or set
> +      that base to "NULL".  */
> +   base = info->base; 
> +   if (base == 0)
> +     return;
> +     
> +   id = frame_id_build (base, func);
> +   (*this_id) = id;  

You're also using a lot of local variables that we usually don't
introduce for this function. I usually don't really care one way
or the other, but this made your function implementation look
different from the others. This is one of these functions that
are implemented in pretty much every -tdep file, so consistency
with the rest if we can would be nice. See i386-tdep.c...

> + static struct value *
> + lm32_frame_prev_register (struct frame_info *this_frame,
> +                             void **this_prologue_cache,
> +                             int regnum)
> + {
> +   struct lm32_unwind_cache *info;
> +   info = lm32_frame_unwind_cache (this_frame, this_prologue_cache);

Minor style nit: Can you add an empty line after your declaration?
We like to have this empty line between the declaration block and
the statements.

> +   /* None found, create a new architecture from the information provided. */

Missing space at the end...

> Index: gdb/lm32-linux-tdep.h

I think that this file should be renamed lm32-tdep.h. There doesn't
seem to be anything that's specific to Linux in that file. For instance,
I see:

> + enum lm32_linux_regs

This should only depend on the CPU, not the OS...

> + /* Fetch the executable load address for the PIC/FDPIC ABI.
> + 	 Return 0 if successful, -1 if not. */
> + int lm32_load_address(struct gdbarch *gdbarch,
> + 		CORE_ADDR *load_addr);

Never actually implemented...

> Index: gdb/lm32-tdep.c

I'm not going to review that file. I assume that the contents should
be very close to what lm32-linux-tdep.c contained. I'll review this
file again once the duplication between lm32-linux-tdep.c and lm32-tdep
is taken care of.

-- 
Joel