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

[Kris Van Hees]

On Wed, Sep 16, 2020 at 12:59:10PM -0700, Eugene Loh wrote:
> 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".

The point is that this is a test and the lvar[n] column shows with the n
value what the ID column *should* show (but might not in case there is a
bug in the DT_LVAR_OFF2ID() macro definition).

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

The utility serves two purposes: it generates data that we verify in a specific
test case to ensure that the stack layout definitions (macros) are correct, and
it serves the purpose to provide me with output that has been helpful in the
development of the stack layout changes (and continues to be helpful for that
as changes are made).  In that sense, it provides the output needed for both
the testcase and for my development purposes.

Your reference to the output being "nonsensical" is obviously subjective, and
if this were a utility that we expect people to make use of for some reason, I
would be perhaps more swayed to change this - but as I explained, it is part
of a testcase and it is a development check tool (admittedly, for me).  There
is no expectation that anyone would bother to analyze the output in terms of
what it means in a more general sense.

In summary: if you are looking at this output and trying to make sense of
it beyond the very obvious which is output that we can verify in a testcase,
then you are looking for something that is not there and is not meant to be
there.  We have other testcases that produce output that really shouldn't be
considered outside the scope of the testcase.

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

There is nothing to be done here.  The print-stack-layout test utility does
exactly what it is designed to do.  Like pretty much all other test triggers
and related executables, there is nothing else here.