[DTrace-devel] [PATCH] Implementation of the exit() action

[Kris Van Hees]

On Wed, Apr 08, 2020 at 10:54:13PM -0700, Eugene Loh wrote:
> On 04/08/2020 09:51 PM, Kris Van Hees wrote:
> 
> > On Wed, Apr 08, 2020 at 08:10:02PM -0700, Eugene Loh wrote:
> >> On 04/08/2020 08:00 AM, Kris Van Hees wrote:
> >>> The exit() action is quite instrumental to the use of DTrace because it
> >>> allows a tracing script to terminate the tracing session.  In order to
> >>> implement it, various improvements needed to be made to the existing
> >>> code.
> >>>
> >>> 1. Support is being added for tracing data elements that are less than
> >>>      8 bytes.  E.g. the exit() action passes a 32-bit value as argument,
> >>>      to be used as return value for dtrace.  To ensure that data items are
> >>>      aligned properly in the data buffer, appropriate padding is inserted
> >>>      into the buffer between the previously stored item and the one we try
> >>>      to store.  E.g. if a 32-bit value was stored at offset 16, and we want
> >>>      to store a 64-bit value next, code will be generated to store a 4-byte
> >>>      zero value as padding at offset 20 so that the 64-bit value will be
> >>>      stored at offset 24 (which is properly aligned) for 8-byte data items.
> >>>
> >>> 2. The dt_rec_add() function has been modified to update the pcb_bufoff
> >>>      value to be the next offset to store data at, and it returns the offset
> >>>      to store the current data item at.  We now also pass in a gap fill
> >>>      function to be used to insert the proper number of padding bytes (all
> >>>      zero).  This means that dt_rec_add() must be called prior to generating
> >>>      the instructions to actually store the data.
> >> Possibly, the alignment and dt_rec_add() stuff -- which is much more
> >> general than exit(), the subject of this patch -- should be its own
> >> commit.  In fact, if the commit history is ever going to get rewritten,
> >> it might even squash such a dt_rec_add() patch back into the patch that
> >> first introduced that function.  (Actually, those two patches are almost
> >> back-to-back!  There is only one other patch between the patch that
> >> introduced dt_rec_add and this patch.)
> > Yes, you are right.  I could do this as 2 commits.  It is quite a bit more
> > involved to do that though given that the code developed incrementally based
> > on what was needed to be able to implement the exit() action.  Splitting it
> > is going to be a fun task of providing an implementation of the modified
> > dt_rec_add() and reworking the existing code to use that and then redo the
> > patch for the exit() action on top of that.
> >
> > But I can see the value of doing it that way - I originally wanted to but
> > decided against it in the interest of time.  But as is often the case, that
> > is not a good reason to not to the right thing :)
> 
> If you like, I can try refactoring the patch.  Let me know.  I'd take 
> into account your responses to my feedback and redo this patch, 
> splitting it into two.  You can look at it, check it's okay, and then 
> squash the first into its predecessor.

Thanks for the offer.  I already had most of the work done by the time I
saw your email.  See the v2 patches I just posted.  Comments welcome.

> Just let me know.  I'm just trying to offload some work from you.  I 
> think I understand this patch well enough.  I'd be interested to see the 
> BEGIN/END stuff (that other email thread) make some progress. Anyhow, 
> your call.

BEGIN/END is going in tomorrow or Friday.