Re: [PATCH,gdb]: ensures that cie ptr of an fda is a cie

[Joel Brobecker <brobecker at adacore dot com>]

> This increases the robustness of gdb, so I think it is worthwhile to add.

Indeed.

> 2011-07-04 Fawzi Mohamed <fawzi.mohamed@nokia.com>
> 
> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure
> 	that cie pointer really points to a cie 

I'm not a specialist of this area of the code, so I can't approve.
However, I'm noticing some style issues (style is project-specific).
Also, please make sure to use the '-p' switch when creating the diff,
so that we can get a little more context for each hunk (see man diff).

> +enum eh_frame_type {
> +    eh_cie_type_id = 1,
> +    eh_fde_type_id = 2,
> +    eh_cie_or_fde_type_id = 3
> +};

The open curly brace should be on the next line, and our indentation
level is 2 characters, thus:

    enum eh_frame_type
    {
      eh_cie_type_id = 1,
      eh_fde_type_id = 2,
      eh_cie_or_fde_type_id = 3
    };

Also, we try to document every type and every function that we add
in GDB. And for types like this, it is often advantageous to also
document each element (not sure in this case). You can have a look
at struct frame_id in frame.h for an example of how we document
our types.

>  static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
>  				     int eh_frame_p,
>                                       struct dwarf2_cie_table *cie_table,
> -                                     struct dwarf2_fde_table *fde_table);
> +                                     struct dwarf2_fde_table *fde_table,
> +                                     enum eh_frame_type entry_type);

The documentation of these function need to be augmented in order
to describe what this extra parameter does.  Same for all the other
functions that you have modified.

> +      gdb_assert ((entry_type & eh_cie_type_id)!=0);

Style: spaces around the != operator.  Also, please provide a comment
explaining why entry_type & eh_cie_type_id can never be zero.  It might
be obvious if already specified in the function documention, but might
not be so obvious. In fact, what is the purpose of this assertion?
It looks like you're validating the value of entry_type to make sure
that it is either a eh_cie_type_id or a eh_cie_or_fde_type_id.
This leads me to believe that the values inside eh_frame_type
are meant to be bit flags. This would be much clearer with documentation
and by using the following syntax:

    enum eh_frame_type
    {
      eh_cie_type_id = 1 << 0,
      eh_fde_type_id = 1 << 1,
      eh_cie_or_fde_type_id = eh_cie_type_id + eh_fde_type_id
    };

(I might also add an `unknown' value that's equal to zero, just
to name that value).

> +      gdb_assert ((entry_type & eh_fde_type_id)!=0);
> +

Same comment.

-- 
Joel