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

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

(To be honest, I'm kind of punting here since I get lost in the 
parser/CTF stuff.)

On 6/8/21 10:19 AM, Kris Van Hees wrote:
> 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.