[DTrace-devel] [PATCH 05/12] Add support for the stack() action

[Kris Van Hees]

On Wed, Jun 09, 2021 at 01:21:37AM -0400, Eugene Loh wrote:
>         diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>         index f7168ffe..e7715aec 100644
>         --- a/libdtrace/dt_cg.c
>         +++ b/libdtrace/dt_cg.c
>         @@ -1333,21 +1333,72 @@ dt_cg_act_speculate(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>          static void
>          dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>          {
>         -       dt_node_t *arg = dnp->dn_args;
>         -#ifdef FIXME
>         -       uint32_t nframes = 0;
>         -#endif
>         +       dtrace_hdl_t    *dtp = pcb->pcb_hdl;
>         +       dt_irlist_t     *dlp = &pcb->pcb_ir;
>         +       dt_regset_t     *drp = pcb->pcb_regs;
>         +       dt_node_t       *arg = dnp->dn_args;
>         +       int             nframes = dtp->dt_options[DTRACEOPT_STACKFRAMES];
>         +       int             skip = 0;
>         +       uint_t          off;
>         +       uint_t          lbl_valid = dt_irlist_label(dlp);
>         +
>         +       /*
>         +        * Legacy default was dtrace_stackframes_default,
>         +        * in kernel file dtrace/dtrace_state.c.
>         +        */
>         +       if (nframes == DTRACEOPT_UNSET)
>         +               nframes = 20;
> 
>     Yuck - an arbitrary constant inline in code.  This should be a define
>     somewhere so you can use a symbolic name here..
> 
> 
> Okay.  Just seemed to make no sense to define a symbol that is used exactly
> once.   The rationale would be that it makes it clear what the number is, but
> the code above does just that:  it sets something to a specific numerical value
> explaining that value.  Splitting this over different files would seem to
> obfuscate things, but let me know if you want it that way.

The convention in DTrace source code has been to place these in dt_open.c,
similar to:

uint_t _dtrace_stkindent = 14;  /* default whitespace indent for stack/ustack */

I personally think those should all become macro definitions instead and be
placed in a dt_default.h or something like that so it is easy to know where to
look for them.  But the advantage is that we can group all compile-time
constants (that are tuneable as a compile-time configuration) in a central
place.

But that is for a later time.