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

[Eugene Loh]

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.

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.

> If I split it out then it is indeed a good idea to just modify the earlier
> patch that added dt_rec_add() and do the implementation with various data
> item sizes at once.  A force-push on the dev branch isn't a big deal for
> patches that are indeed so close together.
>
>> Comment out trace(string)?  I guess that's okay, but not a part of this
>> commit and not mentioned in the log message.  I understand that
>> trace(string) is broken, but what's the point of this here?
> The reason I commented it out is that if a string value were to be used with
> trace() we would actually corrupt our data stream which isn't a good thing at
> all.
Fair enough but, again, in this patch?

Oh yeah:  Copyright dates need updating.