Re: [PATCH v2 3/4] gdb: dwarf2 generic implementation for caching function data

[Joel Brobecker <brobecker@adacore.com>]

On Wed, Dec 28, 2022 at 05:16:03PM +0100, Torbjorn SVENSSON via Gdb-patches wrote:
> 
> On 2022-12-20 22:02, Tom Tromey wrote:
> > > > > > > Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:
> > 
> > > When there is no dwarf2 data for a register, a function can be called
> > > to provide the value of this register.  In some situations, it might
> > > not be trivial to determine the value to return and it would cause a
> > > performance bottleneck to do the computation each time.
> > 
> > Thanks for the patch.
> > 
> > > This patch allows the called function to have a "cache" object that it
> > > can use to store some metadata between calls to reduce the performance
> > > impact of the complex logic.
> > 
> > > The cache object is unique for each function and frame, so if there are
> > > more than one function pointer stored in the dwarf2_frame_cache->reg
> > > array, then the appropriate pointer will be supplied (the type is not
> > > known by the dwarf2 implementation).
> > 
> > Does this ever happen?  If not perhaps a simpler approach would be
> > better.
> 
> Right now; I don't know, but as the fn member in the dwarf2_frame_state_reg
> struct contains one function pointer per register, the architecture allows
> more than one function pointer per frame.
> If we went with a simpler solution, to only have one data block per frame,
> regardless of what function that is "owning" the data, then it could lead to
> nasty surprises if there is some unwinder that expects to be able to use
> more than data type...
> If we move the function pointer from the register scope to the frame scope,
> then I agree that only one data object would be needed.
> If we stick to having the function pointer per register, I could accept to
> have one data block, but somewhere, an assert should added so that the
> wrongful assumption mentioned above would be caught early rather than
> leading to strange bugs. This would mean that the type needs to be stored in
> the dwarf2_frame_cache struct somehow, but the type is currently internal to
> another compile unit.
> This is basically the reason why I went with the decoupled solution that is
> provided in this patch.
> 
> > 
> > > +struct dwarf2_frame_fn_data
> > > +{
> > 
> > New type should have a comment.
> 
> Okay, I'll add comments, but I would like to know if this is the way to go
> or if there should be some alternative implementation before spending more
> time on this.
> 
> > 
> > > +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> > > +		       int regnum);
> > 
> > Shouldn't this use the fn_prev_register typedef?
> 
> Indeed.
> 
> > 
> > > +  void *data;
> > > +  struct dwarf2_frame_fn_data* next;
> > 
> > Wrong placement of the "*", but really a lot of the code isn't following
> > the GNU / gdb style.
> 
> Don't know why the contrib/check_GNU_style.py in the GCC source tree did not
> flag this. Anyway, will be fixed in v3.
> 
> > > +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> > > +				fn_prev_register fn)
> > 
> > Normally new functions get a comment referring to the header where they
> > are declared.
> 
> Can you point me to an example and I will do something similar for these new
> functions if we decide to go this way.

See for instance is_fixed_point_type's declaration in gdbtypes.h:

    /* Return True if TYPE is a TYPE_CODE_FIXED_POINT or if TYPE is
       a range type whose base type is a TYPE_CODE_FIXED_POINT.  */
    extern bool is_fixed_point_type (struct type *type);

... where we provide a description of what the function does, including
information about all the parameters. And then the corresponding
implementation in gdbtypes.c:

    /* See gdbtypes.h.  */

    bool
    is_fixed_point_type (struct type *type)
    {

The goal here is to provide the function's documentation at
the API level, rather than at the implementation level, for
all situations where it is declared like so.

For situations where the function is declared static,
we put the function's documentation next to the implementation,
as there isn't always a declaration.

The "See xxx.h" comment is there to confirm where the function's
documentation can be found.

-- 
Joel