Re: [PATCH/tilegx] tilegx bug fixes & improvements

[Yao Qi <yao@codesourcery.com>]

On 12/06/2012 02:59 PM, Jiong Wang wrote:
> Hi All,
>
>     attachments fix several bugs on tilegx exposed by dejagnu and improve
> tilegx gdb's info register output.
>
>     after these patches, the dejagnu test result improved from:
>
>                   === gdb Summary ===
>
> # of expected passes            19030
> # of unexpected failures        116
>
> to:
>                   === gdb Summary ===
>
> # of expected passes            19100
> # of unexpected failures        61
>

The result looks good.

We don't attach multiple patches into a single mail.  Assuming each of 
your patches is independent, you can e-mail them one by one.  In each 
mail, the ChangeLog entry and description is included, helping people to 
understand the patch.

>
>
> 0001-tilegx-tdep.c-tilegx_push_dummy_call-stack-alignment.patch
>
>
>  From 85696377fc2c76ea51463c5896f63c96a5ace92e Mon Sep 17 00:00:00 2001
> From: Jiong Wang<jiwang@tilera.com>
> Date: Thu, 6 Dec 2012 12:13:03 +0800
> Subject: [PATCH 1/5]     * tilegx-tdep.c (tilegx_push_dummy_call): stack
>   alignment should be 64bit
>
>        for tilegx, when push args on stack, the alignment should be 64bit
> ---
>   gdb/tilegx-tdep.c |   32 ++++++++------------------------
>   1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index 627a470..2f7f253 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -292,7 +292,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
>     int argreg = TILEGX_R0_REGNUM;
>     int i, j;
>     int typelen, slacklen, alignlen;
> -  static const gdb_byte two_zero_words[8] = { 0 };
> +  static const gdb_byte four_zero_words[16] = { 0 };
>
>     /* If struct_return is 1, then the struct return address will
>        consume one argument-passing register.  */
> @@ -324,44 +324,28 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
>       }
>
>     /* Align SP.  */
> -  stack_dest = tilegx_frame_align (gdbarch, stack_dest);
> -
> -  /* Loop backwards through arguments to determine stack alignment.  */
> -  alignlen = 0;
> -
> -  for (j = nargs - 1; j >= i; j--)
> -    {
> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
> -      alignlen += (typelen + 3) & (~3);
> -    }
> -
> -  if (alignlen & 0x4)
> -    stack_dest -= 4;
> +  stack_dest = (stack_dest + 7) & ~0x7;

Your can use function 'align_up'.

>
>     /* Loop backwards through remaining arguments and push them on
>        the stack, word aligned.  */
>     for (j = nargs - 1; j >= i; j--)
>       {x
>         gdb_byte *val;
> -      struct cleanup *back_to;
> -      const gdb_byte *contents = value_contents (args[j]);
>
>         typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
> -      slacklen = ((typelen + 3) & (~3)) - typelen;
> -      val = xmalloc (typelen + slacklen);
> -      back_to = make_cleanup (xfree, val);
> -      memcpy (val, contents, typelen);
> +      slacklen = ((typelen + 7) & (~7)) - typelen;
> +      val = alloca (typelen + slacklen);
> +      memcpy (val, value_contents (args[j]), typelen);
>         memset (val + typelen, 0, slacklen);
>
>         /* Now write data to the stack.  The stack grows downwards.  */
>         stack_dest -= typelen + slacklen;
>         write_memory (stack_dest, val, typelen + slacklen);
> -      do_cleanups (back_to);

alloca is unsafe, and we prefer to use xmalloc/cleanup+xfree.


> 0002-tilegx-tdep.c-tilegx_analyze_prologue-fix-prologue-a.patch
>
>
>  From 028cafa74fe2601ebf13e9a64e6de90e891cbed8 Mon Sep 17 00:00:00 2001
> From: Jiong Wang<jiwang@tilera.com>
> Date: Thu, 6 Dec 2012 12:15:32 +0800
> Subject: [PATCH 2/5]     * tilegx-tdep.c (tilegx_analyze_prologue): fix
>   prologue analysis overflow bug
>
>          when setting breakpoint by function name, if that function
>        is in .so and that .so is not loaded yet, then gdb will do
>        partial name match, which will match the corresponding plt
>        stub entry. We should take this situation into account when
>        doing prologue analysis.
> ---
>   gdb/tilegx-tdep.c |   18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index 2f7f253..93969c9 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -432,8 +432,22 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
>
>   	  status = safe_frame_unwind_memory (next_frame, instbuf_start,
>   					     instbuf, instbuf_size);
> -	  if (status == 0)
> -	    memory_error (status, next_addr);
> +	  if (status == 0) {
> +            /* fix gdb.base/gdb1250
> +             * breakpoint is set before dynamic library loaded, thus gdb
> +             * does a partial symbol name finding and sets the breakpoint
> +             * in the plt stub. our 32-bundle prefetch window is too large
> +             * for this situation to cause a memory access error.
> +             * For plt stub, we just need to return directly.
> +             *
> +             * x86 does not have this problem, because the first instruction
> +             * in their plt stub is jump, which ends the analysis also.
> +             */

The comment style is not gnu style.  The problems looks about plt.  I 
find tilegx port doesn't have a plt stub unwinder.  I am not sure 
creating a plt stub unwinder can fix this problem, but it should fix 
other fails in testsuite.

> +            if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name,
> +                 ".plt") == 0)
> +              return instbuf_start;
> +            memory_error (status, next_addr);
> +          }
>   	}
>
>         reverse_frame_valid = 0;
-- 
Yao (齐尧)