[DTrace-devel] [PATCH 03/14] Promote associative integer arrays to 64 bits when loaded

[Eugene Loh]

On 5/4/23 13:10, Eugene Loh wrote:

> On 5/3/23 23:54, Kris Van Hees wrote:
>
>> On Mon, May 01, 2023 at 11:47:11PM -0400, eugene.loh--- via 
>> DTrace-devel wrote:
>>> From: Eugene Loh <eugene.loh at oracle.com>
>>>
>>> In commit 7e9ce1eee475 ("Promote integers to 64 bits when loaded"),
>>> integers were promoted to 64 bits when loaded.  Associative arrays
>>> were on a different code path.
>>>
>>> Add the promotion to the associative-array code path and add tests
>>> for different variable types.  Also, do some minor refactoring in
>>> dt_cg_assoc_op() in anticipation of subsequent refactoring; this
>>> prep work is done here to improve readability of subsequent patches.
>> OK on the addition of the promotion.
>>
>>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>>> @@ -3988,7 +3988,7 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t 
>>> *dlp, dt_regset_t *drp)
>>>   }
>>>     static void
>>> -dt_cg_assoc_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>> +dt_cg_assoc_op(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
>> I do not really agree with this change (and the resulting changes 
>> below)...
>> This is a function that generates code for a node, and the convention 
>> in the
>> code generator is that dnp is used for that variable.  The change to 
>> 'dst'
>> seems to imply that this node is the destination of some operation, 
>> but then
>> one would expect there to be a source as well, and that is clearly 
>> not the
>> case.

Actually, dt_cg_assoc_op() has only one call site, and that's also where 
the only dt_cg_load_var() call site is.  And dt_cg_load_var() uses "dst" 
rather than "dnp" -- that is, "dst" is being used for gvars, lvars, and 
tvars.  So using "dst" also for assoc is not much of a stretch.  In 
particular, the roadmap is for dt_cg_assoc_op() to be swallowed up by 
dt_cg_load_var(), which is the whole motivation for this patch.  Even 
the "promote" change is so motivated:  the promote bug was discovered by 
comparing dt_cg_assoc_op() to dt_cg_load_var().

Nevertheless, here is a different suggestion.  How about I remove the 
dnp-to-dst renaming from this patch -- making this patch exclusively 
about the promote bug -- and leave the renaming until the later patch 
("Inline assoc_op() into load_var()"), where the consistent dnp/dst 
naming is really needed?  I had wanted to keep the variable renaming and 
the big code movement in different patches to make it easier to see 
what's going on, but maybe that was misguided.

Let me know.

>> Let's keep this as-is, please.
>
> I hear you, but this change isn't meant to stand on its own.  It's 
> setting up some code refactoring... in essence, the code being 
> modified here is going to disappear in later patches.  The changes in 
> this patch are supposed to be easy to review ("Oh yeah, I can see he's 
> simply doing a simple renaming") and trickier stuff will be in later 
> patches.
>
> Ultimately, the point is to combine tvar and assoc access in a way 
> that common code can be extracted to simplify the code and make it 
> easier to make changes (e.g., to support NULL strings).
>
> So I get your point on this patch in isolation, but I do not 
> understand what you're suggesting for the overall roadmap.  I would 
> think combining tvar and assoc more is unequivocally a good thing.
>