[DTrace-devel] [PATCH v2 03/19] Deprecate enabled probe ID (epid)

[Eugene Loh]

On 9/9/24 15:10, Kris Van Hees wrote:

> On Mon, Sep 09, 2024 at 02:40:34PM -0400, Eugene Loh wrote:
>> On 9/9/24 13:31, Kris Van Hees wrote:
>>
>>> On Sun, Sep 08, 2024 at 12:31:12PM -0400, Eugene Loh wrote:
>>>> On 9/5/24 23:31, Kris Van Hees wrote:
>>>>> On Tue, Sep 03, 2024 at 01:16:43AM -0400, eugene.loh at oracle.com wrote:
>>>>>> From: Eugene Loh <eugene.loh at oracle.com>
>>>>>> Deprecate the use of EPID:
>>>>>>
>>>>>> *)  So we rearrange as follows:
>>>>>>
>>>>>>                old                        new
>>>>>>
>>>>>>             0: pad (size)                 pad (size) <= buffer start
>>>>>>             4: pad (additional)           specid
>>>>>>             8: epid <= buffer start       prid
>>>>>>            12: specid                     stid
>>>>>>            16: data[n]                    data[n]
>>>>>>
>>>>>>        So now, we say there is no longer an 8-byte pad before the
>>>>>>        buffer start;  rather, the buffer starts with a 4-byte pad.
>>>>> I am not sure whether this layout comparison is comprehensible to anyone not
>>>>> familiar with this code.
>>>> Yeah.  Incidentally, I had moved specid so that prid/stid were in the same
>>>> 8-byte block so that someone could read it as a single 8-byte EPID if they
>>>> wanted to.  I'm assuming you see no value in that.  So I moved specid back
>>>> to where it used to be and I think this simplifies the description of what's
>>>> going on.  Anyhow, I revised this explanation.  Hopefully it's sufficient
>>>> now.
>>> I like the specid first actually :)  Since it reflects the designation of the
>>> data.
>> The most recent version of the patch has prid, stid, specid.  Is that fine,
>> or do you want another version that reorders these things?  It seems to me
>> that at some point we should converge on some version of this stuff and
>> worry less about second-order considerations of things that do not impact
>> the user.  Anyhow, I can make the change.  Just tell me in what order you
>> want these things or whether the most recent version is fine.
> I was just wondering why you changed the order again, since that wasn't part
> of the review feedback.  It just seemed odd, because the moving of specid to
> the beginning of the buffer actually made total sense, so reversing it seemed
> odd.
>
> I agree 100% on the need to converge, which is why such a change seemed rather
> unusual to me.

Fair enough.

FWIW, it seemed to me that returning specid to its original offset was a 
simpler way of getting to "consistent order" (between comments, layout, 
and cg).  And, again, the only reason (in my mind) for having moved 
specid in the first place was so that prid/stid could coexist in the 
same 8-byte (aligned) spot, which is no longer important.  To me, there 
was no special reason to have specid first.

Anyhow, what's the next step?  Shall I revise the patch for specid, 
prid, stid ordering?

>>>>>> diff --git a/test/unittest/actions/setopt/tst.badopt.r b/test/unittest/actions/setopt/tst.badopt.r
>>>>>> @@ -1,16 +1,16 @@
>>>>>>     -- @@stderr --
>>>>>> -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
>>>>>> +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
>>>>> Perhaps it would be more efficient to update runtest.sh to support both the
>>>>> old and the new error reporting output, and changing both into a single unified
>>>>> form that substitutes changeable elements.
>>>> I don't get this.  First, there is no reason to support the old output...
>>>> that would be a rather remarkable historical vestige to keep in the code
>>>> base.  Second, it seems simpler and more stringent just to compare to a
>>>> results file in some simple, straightforward way.  You say "perhaps" so I
>>>> just left it as is.
>>> I was more thinking about making the output we compare less dependent on how
>>> things are phrased so that we do not have to update .r files whenever there is
>>> a change in how we output the data.  Except for a test or two that verify the
>>> exact phrasing we expect.
>> Okay, but again it seems to me that there are multiple acceptable ways of
>> doing these things, each way with its own pros and cons. So, do you want
>> another version of this patch with the requested behavior, or can we move
>> forward with the patch as it was most recently posted?  There are other ways
>> in which the format of DTrace output could be converted to some "canonical"
>> form before checking; do we want to embark on such a project now, or first
>> get USDT functionality out the door to users?  I vote for worrying less
>> about some of this "under the hood" fine tuning (that doesn't even have
>> unequivocal right/wrong) and more about delivering features to users.
>>
>> Anyhow, let me know.  I can make the requested change, but am nervous that
>> we're churning on issues that do not benefit users.
> We are not.  But I would be remiss if I didn't mention areas where we can
> improve things, especially if it would result in less extensive patches for
> things that in the end do not really matter much in terms of what is being
> tested.
>
> As you pointed out, I phrased it originally as 'Perhaps' - this is certainly
> not a make-or-break part of the patch.  Then again, I had also suggested
> leaving the changing of the error output as a post-epid-deprecation thing.
>
> One way or another, we should have an action item for this so changes to the
> error output do not cause such extensive patches.

Okay, so where are we on this one?  I confess I don't really understand 
what, specifically, you are suggesting for runtest.sh. E.g., would we 
check the specific dt_clause_n ID?  (The "n" value.) What canonical form 
are you suggesting for the output?

>>>>> E.g.
>>>>> dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
>>>>>
>>>>> and
>>>>>
>>>>> dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
>>>>>
>>>>> could both be rewritten using pattern amtching and substitutions to be:
>>>>>
>>>>> dtrace: error for probe (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
>>>>>
>>>>> and tests that are supposed to exercise specific aspects of the error message
>>>>> or that depend on specific details could test that explicitly?