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

[Eugene Loh]

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.

>>>> 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.

>>> 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?