[DTrace-devel] [PATCH 02/20] Fix alignment of global and local variables

[Kris Van Hees]

On Tue, Jun 08, 2021 at 02:16:29AM -0400, Eugene Loh wrote:
> I'll try to look at this more, but don't really understand this stuff.  
> A few initial comments and questions, though.
> 
> dt_cook_op1() still has a set_storage() call with 8-byte alignment.  
> Does it need to be updated?

That is actually correct because it pertains to a case where the variable is
defaulted to type int64_t which is an 8-byte aligned type.  But, perhaps it
is more clear if I do actually assign the type size to a variable and use
that for both the alignment and datatype size in the set_storage call.  No
cost to doing that and if it helps to understand the code, all the better.

> In dt_cook_op2(), there is an "alignment = 9;".  Maybe that's right, but 
> I'm used to seeing 8.  It's obviously right or wrong... just too tired 
> right now to figure out which.

Oh, that is a nasty typo.  Thanks for catching that.

> The two tests test/unittest/variables/lvar/tst.alignment-struct[-2].sh say
> ASSERTION: Variables of array type should be aligned based on their 
> element datatype
> Array type?  Is that a cut-and-paste vestige that should say struct?
> 
> In general, any way of reducing all that redundancy in the new tests?  
> E.g., so that if someone wants to make a small change, they don't have 
> to go through all those nearly redundant files?

Not really - that is the unfortuante side effect of extensive unit tests.
There is probably a way we could somehow auto-generate the tests on the fly,
or use some include-file magic to centralize the common elements.  Many areas
in the testsuite could be improved that way but unfortunately the priority
tends to remain on development of features and fixing bugs.

Improvements to tests are always welcome, but are likely to always get done
in follow-up patches so as to not impact development flow too much.

> On 6/2/21 1:47 AM, Kris Van Hees wrote:
> > Storage for global and local variables was allocated at a 8-byte
> > boundary regardless of the dattype of the variables.  We now use
> > the proper alignment for each specific variable based on its
> > datatype.
> 
> dattype -> datatype

Thanks.

> > This patch also corrects the expected output for the tst.basics.d
> > arithmetic test.
> 
> Can you remind me why?  The test used to behave like DTv1.  Now like 
> GCC.  So, DTv1 was wrong?

Well, DTrace v1 clearly got it wrong.  It is known to have some cases where
the datatype of the variable isn't actually considered and scalars are used
as plain 64-bit entities.  This seems to be one of those cases.