Re: [PATCH v3 1/1] Don't rewind PC for GHC generated frames

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Bartosz,

Thanks for the additional info in the commit message, it's very useful.

I hoped there would be some comments from others.  In particular, is 
anybody able to tell if adding a call to find_pc_compunit_symtab in 
get_frame_address_in_block is a performance concern?  How frequently is 
get_frame_address_in_block called, and how costly is 
find_pc_compunit_symtab to call?

I noted two small comments, no need to submit another version just for 
that, we can fix it before pushing.  However, please tell me if you are 
fine with my suggestion below.

> diff --git a/gdb/frame.c b/gdb/frame.c
> index 1384ecca4f..9ff0dcb130 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2458,7 +2458,14 @@ get_frame_address_in_block (struct frame_info
> *this_frame)
>        && (get_frame_type (this_frame) == NORMAL_FRAME
>  	  || get_frame_type (this_frame) == TAILCALL_FRAME
>  	  || get_frame_type (this_frame) == INLINE_FRAME))
> -    return pc - 1;
> +    {
> +      /* GHC intermixes metadata (info tables) with code, going back 
> is
> +         guaranteed to land us in the metadata.  */

I think the main reason why we don't want to subtract one is that the 
compiler generates return addresses that refer to the jump instruction 
that made the "call", and not to the instruction after, as with the call 
instruction.  I think the comment should reflect that.  What about:

/* GHC (Glasgow Haskell Compiler) uses jump and manages its own stack 
rather than
    using the call instruction.  The return address it generates is that 
of the
    jump instruction, not the following instruction.  It is therefore not
    necessary to go back one byte.  It would not be desirable anyway, 
because it
    intermixes metadat (info tables) with code, so going back would land 
us in the
    metadata.  */

> +      struct compunit_symtab *cust = find_pc_compunit_symtab (pc);
> +      if (cust != NULL && cust->producer_is_ghc)
> +        return pc;
> +      return pc - 1;
> +    }
> 
>    return pc;
>  }
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index f9d52e7697..c164e5ba5f 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1432,6 +1432,9 @@ struct compunit_symtab
>       instruction).  This is supported by GCC since 4.5.0.  */
>    unsigned int epilogue_unwind_valid : 1;
> 
> +  /* This CU was produced by Glasgow Haskell Compiler */

The comment should end with a period and two spaces.

Thanks,

Simon