Re: [DTrace-devel] [PATCH 3/3] cg: optimize ternary expressions for strings

[Kris Van Hees <kris.van.hees@oracle.com>]

On Wed, Jul 16, 2025 at 01:00:43AM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> subject to a few comments below.
> 
> On 7/15/25 15:50, Kris Van Hees via DTrace-devel wrote:
> 
> > If either size of a ternary expression has a tstring value, it can be
> 
> s/size/side/

Thanks.

> > re-used to store the value of the ternary expression, reducing the
> > need for tstring allocation, especially in nested ternaries.
> > 
> > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > @@ -222,7 +222,7 @@ typedef struct dt_kern_path {
> >    * - cleanpath() holds a prepended '/' char, a string, an appended '/' char,
> >    *   and a terminating NUL char, or STRSZ + 3 chars altogether
> >    */
> > -#define DT_TSTRING_SLOTS	4
> > +#define DT_TSTRING_SLOTS	6
> 
> Same comment as I had for Alan's patch.  The comment block before this goes
> into excruciating detail about why the value should be 4. Whether its logic
> is right or wrong, we cannot leave those old comments with a new value.  Why
> is the value now 6?

Ah yes, I should have updated the explanation.  It has become a little bit more
complicated but I can certainly explain it based on the example we have in the
testsuite.
> 
> I assume 6 is not a "sufficient for all purposes" value.  E.g., if I kick up
> the complexity on tst.tstring_ternary_nested.d, I get:

Correct - I hope to do a later patch that makes this something that adjusts
dynamically.  But that is quite a bit more work.
> 
> $ git diff
> diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d
> b/test/unittest/codegen/tst.tstring_ternary_nested.d
> @@ -16,6 +16,8 @@ BEGIN {
>         trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
>               x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
>               x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> +             x > 4 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> +             x > 5 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
>               strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));
> 
>         exit(0);
> $ sudo ./runtest.sh test/unittest/codegen/tst.tstring_ternary_nested.d
> test/unittest/codegen/tst.tstring_ternary_nested.d: FAIL: core dumped.
> 1 cases (0 PASS, 1 FAIL, 0 XPASS, 0 XFAIL, 0 SKIP)
> $ cat test/log/current/runtest.log
> [...]
> dtrace: libdtrace/dt_cg.c:1472: dt_cg_tstring_xalloc: Assertion `i <
> DT_TSTRING_SLOTS' failed.
> 
> > diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d b/test/unittest/codegen/tst.tstring_ternary_nested.d
> > @@ -0,0 +1,26 @@
> > +BEGIN {
> > +	x = 42;
> > +	trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
> > +	      x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
> > +	      x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> > +	      strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));
> > +
> > +	exit(0);
> > +}
> 
> I guess that's fine for cg, but the skeptic in me notices that the three
> tests all evaluate to true.  I would think one could run through all 2x2x2=8
> cases in a single test for a little bit more rigor.

Since this is a codegen test, we only really care about the tstring allocation
aspect.  There are already other tests that exercise the ternary itself.