[DTrace-devel] [PATCH] Fix stack layout definitions to match the new dt_dctx design

[Eugene Loh]

I like the changes that are in v2 of the patch.  One remaining point, 
and I figured it would make more sense to reply to this old email rather 
than to the v2 patch...

On 09/04/2020 12:18 AM, Kris Van Hees wrote:

> On Thu, Sep 03, 2020 at 10:25:59PM -0700, Eugene Loh wrote:
>> As far as the test code goes, the lvar stuff is pretty confusing. The .r
>> file has:
>>        lvar[ -1]:  -103 (ID  -1)
>> lvar[  0]:  -104 (ID   0)
>> lvar[  1]:  -112 (ID   1)
>> lvar[ 19]:  -256 (ID  19)
>> lvar[ -1]:  -257 (ID  -1)
>>
>> Wait, what?  lvar[-1] is -103?  Or lvar[-1] is -257?  Actually, neither
>> one makes sense.  My recommendation is to have the test just do
>> something like
>>
>>       #define TESTLVARID(off) printf("ID(% 4d) = % 3d\n", off,
>> DT_LVAR_OFF2ID(off));
>>       TESTLVARID(DT_STK_LVAR(0) + 1)
>>       TESTLVARID(DT_STK_LVAR(0))
>>       TESTLVARID(DT_STK_LVAR(1))
>>       TESTLVARID(DT_STK_LVAR(DT_LVAR_MAX))
>>       TESTLVARID(DT_STK_LVAR(DT_LVAR_MAX) - 1)
>>
>> That is, do not report the first column (which is really just for
>> correctness checking, but all we actually do is check against the .r
>> file) and just focus on going from offset to ID.
> The point is that the print-stack-layout little prog is used by the test but
> it exists because it makes it easy to see what the constant values are and it
> also provides minimal testing of some of the boundary conditions for variable
> offsets.  The output may look rather weird but there is actually a purpose
> behind it.  What we're really testing is that the offset of the byte before the
> lvar storage and the offset of the byte after the lvar storage yield an invalid
> lvar (-1) condition.

Right.  The underlying intentions sound good.

But the point is that the reader is left trying to figure out what 
"lvar[-1]" means, when that expression is at best nonsensical but more 
realistically wrong.  After all, DT_STK_LVAR(-1) is -96 rather than -103 
and -257.  I think what you're trying to say is that the ID is -1, but 
that is already being output as the clearly labeled "ID".

Simply removing that puzzling column in the output gets rid of this 
nonsensical reference without at all diminishing the rigor (or any other 
quality) of the test.  It would simply make the output clearer.  And if 
one did not have to generate that spurious output, the utility source 
code would be more compact and clearer.

> Sure, the output could be made more clear or polished but that is hardly an
> issue we need to address.

I'm puzzled.  The meat of this patch is to clarify comments and 
explanations.  "More clear or polished" is what this patch is basically 
about.

If you're open to the output being more clear or polished but don't have 
the time, let me know and I can post an amended patch.

>
>> On 08/27/2020 11:54 AM, Kris Van Hees wrote:
>>> diff --git a/test/unittest/codegen/tst.stack_layout.r b/test/unittest/codegen/tst.stack_layout.r
>>> -lvar[ -1]:   -87 (ID  -1)
>>> -lvar[  0]:   -88 (ID   0)
>>> -lvar[  1]:   -96 (ID   1)
>>> -lvar[ 21]:  -256 (ID  21)
>>> +lvar[ -1]:  -103 (ID  -1)
>>> +lvar[  0]:  -104 (ID   0)
>>> +lvar[  1]:  -112 (ID   1)
>>> +lvar[ 19]:  -256 (ID  19)
>>>    lvar[ -1]:  -257 (ID  -1)
>>>    scratch:    -257 .. -512