Re: [PATCH] Cleanup `objfile' from probe interface

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Wed, 18 Jul 2012 17:02:20 +0200, Sergio Durigan Junior wrote:
> The patch removes several explict references to OBJFILE from many
> functions in the current probe API, by internalizing the OBJFILE on
> `struct probe'.  This also simplified `struct sym_probe_fns'.

insert_exception_resume_from_probe still needs this cleanup, doesn't it?


> @@ -297,7 +279,7 @@ collect_probes (char *objname, char *provider, char *probe_name,
>        struct probe *probe;
>        int ix;
>  
> -      if (! objfile->sf || ! objfile->sf->sym_probe_fns)
> +      if (!objfile->sf || !objfile->sf->sym_probe_fns)
>  	continue;
>  
>        if (objname)

This is unrelated formatting change but let it be that way...


> @@ -334,26 +312,26 @@ collect_probes (char *objname, char *provider, char *probe_name,
>    return result;
>  }
>  
> -/* A qsort comparison function for probe_and_objfile_s objects.  */
> +/* A qsort comparison function for probe_p objects.  */
>  
>  static int
>  compare_entries (const void *a, const void *b)

Rather compare_probes now.

There are some more variables called entries/entry which could be probes/probe
now but that is more a nitpick.


> @@ -440,7 +417,7 @@ gen_ui_out_table_header_info (VEC (probe_and_objfile_s) *probes,
>     represented by ENTRY.  */

represented by PROBE.


>  
>  static void
> -print_ui_out_info (probe_and_objfile_s *entry)
> +print_ui_out_info (struct probe *probe)
>  {
>    int ix;
>    int j = 0;
[...]
> @@ -118,7 +115,6 @@ struct probe_ops
>         position in the vector.  */
>  
>      void (*gen_info_probes_table_values) (struct probe *probe,
> -					  struct objfile *objfile,
>  					  VEC (const_char_ptr) **values);

Leftover 'OBJFILE' in its comment:
    /* Function that will fill VALUES with the values of the extra fields
       to be printed for PROBE  and OBJFILE.  If the backend implements


>    };
>  
> @@ -157,6 +153,9 @@ struct probe
>      /* The operations associated with this probe.  */
>      const struct probe_ops *pops;
>  
> +    /* The objfile related to this probe.  */

Isn't it rather s/related/containing/?  I would also state that if the same
probe is contained also in the separate debug objfile this variable contains
always the non-separate-debuginfo objfile.


> +    struct objfile *objfile;
> +
>      /* The name of the probe.  */
>      const char *name;
>  
[...]
> @@ -1159,35 +1158,40 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
>  {
>    CORE_ADDR pc = expr->scope;
>    int sel = (int) (uintptr_t) data;
> -  struct objfile *objfile;
>    struct probe *pc_probe;
> -  int n_probes;
> +  const struct sym_probe_fns *pc_probe_fns;
> +  int n_args;
>  
>    /* SEL == -1 means "_probe_argc".  */
>    gdb_assert (sel >= -1);
>  
> -  pc_probe = find_probe_by_pc (pc, &objfile);
> +  pc_probe = find_probe_by_pc (pc);
>    if (pc_probe == NULL)
>      error (_("No SystemTap probe at PC %s"), core_addr_to_string (pc));
>  
> -  n_probes
> -    = objfile->sf->sym_probe_fns->sym_get_probe_argument_count (objfile,
> -								pc_probe);
> +  gdb_assert (pc_probe->objfile != NULL);
> +  gdb_assert (pc_probe->objfile->sf != NULL);
> +  gdb_assert (pc_probe->objfile->sf->sym_probe_fns != NULL);
> +
> +  pc_probe_fns = pc_probe->objfile->sf->sym_probe_fns;
> +
> +  n_args = pc_probe_fns->sym_get_probe_argument_count (pc_probe);
> +
>    if (sel == -1)
>      {
>        value->kind = axs_rvalue;
>        value->type = builtin_type (expr->gdbarch)->builtin_int;
> -      ax_const_l (expr, n_probes);
> +      ax_const_l (expr, n_args);

This rename is really a separate cleanup.


>        return;
>      }
>  
>    gdb_assert (sel >= 0);
> -  if (sel >= n_probes)
> +  if (sel >= n_args)
>      error (_("Invalid probe argument %d -- probe has %d arguments available"),
> -	   sel, n_probes);
> +	   sel, n_args);
>  
> -  objfile->sf->sym_probe_fns->sym_compile_to_ax (objfile, pc_probe,
> -						 expr, value, sel);
> +  pc_probe_fns->sym_compile_to_ax (pc_probe, expr, value,
> +				   sel);

This can be on a single line.


>  }
>  
>  \f


OK with those changes, it is pre-approved that way.


Thanks,
Jan