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

[Kris Van Hees]

On Thu, Mar 17, 2022 at 08:26:16PM -0400, Eugene Loh via DTrace-devel wrote:
> I made many of the changes but am curious about one thing:
> 
> On 3/17/22 7:15 PM, Eugene Loh wrote:
> > On 3/17/22 6:45 PM, Kris Van Hees wrote:
> > > I am not sure about this patch...  First of all, passing the isptr
> > > argument
> > > seems unnecessary because the type of dnp should give us that info
> > > already.
> > Okay.  I'll try to eliminate isptr.
> > > 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).  So, one would expect that code calling the
fucntion adheres to that prototype.  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).

> I'm only talking about the types of those first args.  I agree with you on
> the return values:  they should all be void and I've updated the comments
> accordingly.
> > > And ensure that they all have the same return type.
> > Got it.
> > > Then, with the comment describing the function updated, this seems to be
> > > both useful and consistent.
> > Okay.  I'll clean up the patch series and repost.  Thanks for the
> > comments.
> > > On Thu, Mar 17, 2022 at 04:37:49PM -0400, eugene.loh--- via
> > > DTrace-devel wrote:
> > > > From: Eugene Loh <eugene.loh at oracle.com>
> > > > 
> > > > When the code generator sets up a call to a precompiled BPF function,
> > > > it emits a lot of boilerplate code.  The implementations of DTrace
> > > > subroutines typically make such calls.  So there is potential for
> > > > much code reuse.
> > > > 
> > > > The "path" helper is one such attempt.  Since its usefulness is
> > > > broader than just for a "path name" argument, rename this helper to
> > > > suggest its broader usage more clearly.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel