[DTrace-devel] [PATCH 2/5] slight code refactoring to eliminate gratuitous differences between providers

[Eugene Loh]

On 06/15/2020 02:59 PM, Kris Van Hees wrote:

> On Mon, Jun 15, 2020 at 09:41:10AM -0700, Eugene Loh wrote:
>> On 06/12/2020 06:03 PM, Kris Van Hees wrote:
>>
>>> On Thu, Jun 11, 2020 at 05:29:38PM -0400, eugene.loh at oracle.com wrote:
>>>> diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
>>>> @@ -40,19 +40,6 @@ static const char		modname[] = "vmlinux";
>>>>    
>>>>    #define SYSCALLSFS		EVENTSFS "syscalls/"
>>>>    
>>>> -/*
>>>> - * We need to skip over an extra field: __syscall_nr.
>>>> - */
>>>> -#define SKIP_EXTRA_FIELDS	1
>>>> -
>>>> -static const dtrace_pattr_t	pattr = {
>>>> -{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>>>> -{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
>>>> -{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>>>> -{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>>>> -{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>>>> -};
>>>> -
>>>>    struct syscall_data {
>>>>    	dt_pt_regs	*regs;
>>>>    	long		syscall_nr;
>>>> @@ -67,6 +54,19 @@ struct syscall_data {
>>>>    #define ENTRY_PREFIX	"sys_enter_"
>>>>    #define EXIT_PREFIX	"sys_exit_"
>>>>    
>>>> +/*
>>>> + * We need to skip over an extra field: __syscall_nr.
>>>> + */
>>>> +#define SKIP_EXTRA_FIELDS	1
>>>> +
>>>> +static const dtrace_pattr_t	pattr = {
>>>> +{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>>>> +{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
>>>> +{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>>>> +{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>>>> +{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>>>> +};
>>>> +
>>>>    /* Scan the PROBE_LIST file and add probes for any syscalls events. */
>>>>    static int populate(dtrace_hdl_t *dtp)
>>>>    {
>>> I do not understand the rationale for the change.  I can think of some
>>> re-ordering that could be done, but not what you propose here,  Cam you
>>> explain why you chose to to this?
>> Sure.  There is a pattern of overall organization that is evident in the
>> various dt_prov_*.c files, with one exception being the placement of the
>> stability attributes in this one file.  There is no justification (that
>> I know of) for that one egregious exception; meanwhile, uniform
>> organization would be nice for developers.
>>
>> Incidentally, here is that overall organization:
>>
>>       +---------------  dt_prov_dtrace.c
>>       | +-------------  dt_prov_fbt.c
>>       | | +-----------  dt_prov_profile.c
>>       | | | +---------  dt_prov_sdt.c
>>       | | | | +-------  dt_prov_syscall.c
>>       | | | | |
>>       | | | | |
>>       v v v v v
>>
>>       * * * * *  Copyright notice and introductory comments
>>       * * * * *  include files
>>       * * * * *  define names like prvname, modname, funname
>>               *  STABILITY ATTRIBUTES
>>       * * * * *  define preprocessor symbols (tracefs files, prefixes,
>> suffixes)
>>       * * * *    STABILTIY ATTRIBUTES
>>       * * * * *  populate()
>>       * * * * *  trampoline()
>>       * * * * *  probe_info()
>>           *      provide()
>>       * *        probe_fini()
>>       * * * * *  dt_provimpl_t = { }
>>
>> You can see that moving the stability attributes in that one file
>> improves the uniformity.
> Well, yes and no.  You focus on the stability attributes

The focus is not on the stability attributes per se.  It's on the coarse 
organization of the file.  You say you do not see the rationale for 
fixing that.  I don't get it.

> but ignore the bulk
> of the defines that are also not uniform.  If we are going to bring uniformity
> to these things, lets apply that to all rather than just one thing?

It's not just one thing.  There were a variety of things in the patch.

Now, if you think the #defines should be more uniform, let me know what 
you have in mind.  Currently, the different providers use different 
symbols.  E.g., sdt watches for various probes that it will exclude;  no 
other provider does such a thing.  It has symbols related to reading the 
"format" file; no other provider does since they all use sdt's.  The 
dtrace provider needs to set up some uprobes, which the other providers 
do not.  The syscall provider has to handle scd args, which the other 
providers do not.  The profile provider has two kinds of probes 
(profile- and tick-) that legacy DTrace handled with macro symbols;  
that can be changed but in any case it's different from what the other 
providers do.  Etc.

So, what uniformity, exactly, do you expect to find among the #defines?

They do all use DCTX_FP(), but it's already embedded in the trampoline() 
in all cases.

>> (Not all providers have provide() or probe_fini() functions.  Also, the
>> inclusion of the profile provider is a little anachronistic -- it
>> appeared in a later commit -- so you can omit that column if you like.)