[DTrace-devel] [PATCH 1/3] Rename "path" helper to "tstring1" helper

[Eugene Loh]

On 3/17/22 8:46 PM, Kris Van Hees wrote:

> On Thu, Mar 17, 2022 at 08:26:16PM -0400, Eugene Loh via DTrace-devel wrote:
>> On 3/17/22 7:15 PM, Eugene Loh wrote:
>>> On 3/17/22 6:45 PM, Kris Van Hees wrote:
>>>> But more importantly, you are now using this helper function to make BPF
>>>> calls to functions with differnet (incompatible) prototypes (once
>>>> the other
>>>> patches are applied also):
>>>>
>>>>      void dt_basename(char *src, char *dst)
>>>>      void dt_dirname(char *src, char *dst)
>>>>      int dt_lltostr(uint64_t VAL, char *STR)
>>>>      uint64_t dt_inet_ntoa(uint8_t *src, char *dst)
>>> Ugh.  Both dt_lltostr() and dt_inet_ntoa() are actually void. I'll fix
>>> those comments (which is all they are) in their respective patches.
>>>> And the comment block before the helper states:
>>>>
>>>>       /*
>>>>        * This function is a helper function for subroutines that take one
>>>>        * argument and return the tstring resulting from applying a
>>>> given BPF
>>>>        * function (passed by name) on it.
>>>>        */
>>>>
>>>> So, there is quite some inconsistency here.  None of the functions
>>>> you use
>>>> this helper for take just one argument.  None of them return a tstring.
>>>> And both their argument lists vary (well, the first argument), and the
>>>> return type is different.
>>>>
>>>> While it does "work", I do not thiink it is appropriate to do things
>>>> this
>>>> way.  At a minimum, the first argument should probably be uintptr_t
>>>> so that
>>>> you properly capture that it is a value that can be a pointer or an
>>>> integer.
>> Actually, I don't really get this.  There is no type checking.  And the code
>> generation to call the precompiled BPF function is the same regardless of
>> whether that first arg is a pointer or integer or whatever.  (Well, modulo
>> that isptr thing you pointed out.)  The "prototypes" we're talking about
>> here are just comments in the various bpf/*.S files.  Changing the types of
>> the first args from their current types to uintptr_t -- again, we're only
>> talking about changes in comments -- would make those comments less clear.
> The comment presumably would describe the function prototype (and if it does
> not then why is it there).
Right.  So the comment in the bpf/*.[c|S] file should say what the arg is.
> So, one would expect that code calling the
> fucntion adheres to that prototype.
The helper is just generating BPF code that pretty much "agnostically" 
moves contents from one BPF reg to %r1.

I just posted v2 of the patch series.  It handles this particular issue 
by making the comment about the %r1 arg in the helper function rather 
than replicating comments in the various bpf/* files saying "you can 
think of the prototype as being this but it's really that."  I hope the 
v2 solution seems reasonable.  Let me know.
> Yes, we do not have type checking for
> these things but when you read through the code and follow call chains, you
> should be able to reasonably assume that these things match.
>
> I knoe I will eventually forget what is going on here and it will be confusing.
>
>> So would you be okay with leaving the types of the first args, as described
>> in the comments, as varying?
> You can just show the prototype of the function using uintptr_t and explain
> (with or without pseudo-code) that for this particular function the argument
> will always be interpreted as an int (or string, for those that do).