Re: [PATCH] Adding support for reding signal handler frame in AIX

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Sangamesh Mallayya wrote:

> I have modified the patch and implemented it using the trad frame cache.
> Please review and let me know the comments.

This version now looks pretty good to me, I just have a few minor comments
related to coding style.  Once those are addresses, it should be ready to
commit.

> +   sigconext structure have the mstsave saved under the

Typo "sigcontext" ?

> +  if (wordsize == 4) {
> +    func = read_memory_unsigned_integer (base_orig +
> +					 SIG_FRAME_PC_OFFSET + 8, 
> +					 tdep->wordsize, byte_order);
> +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8,
> +			      wordsize, byte_order, &backchain);
> +    base = (CORE_ADDR)backchain;
> +  } else {
> +    func = read_memory_unsigned_integer (base_orig +
> +					 SIG_FRAME_LR_OFFSET64,
> +					 tdep->wordsize, byte_order); 
> +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64,
> +			      wordsize, byte_order, &backchain);
> +    base = (CORE_ADDR)backchain;
> +  }

GNU coding style is to put braces on separate lines, and indent like so

     if (wordsize == 4)
       {
         func = ...
       }
     else
       {
         func = ...
       }

Also, why do you use tdep->wordsize in some places and wordsize in others?

> +  if (wordsize == 4) {
> +    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
> +                             base_orig + 0x38 + 52 + 8);
> +  } else {
> +    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
> +                             base_orig + 0x70 + 320);
> +  }

Where there is just a single line inside the if / else block, you
should just omit the braces completely.

>	* gdb.base/aix-sighandle_test.c: New file.
>	* gdb.base/aix-sighandle_test.exp: New file.

Those should best go in gdb.arch, not in gdb.base (since they are
specific to a single platform).   Also, it may be better to omit
the "_test" part of the file names (all files in these directories
are tests!).

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com