[PATCH 2/3] cg: validate tstring alloc/free

[PATCH 2/3] cg: validate tstring alloc/free

[Kris Van Hees]

Rather than undiscriminantly resetting tstring allocations at the
beginning of a compilation, actually verify that alloc/free of
tstrings is done correctly, i.e. that none are left allocated after
compilation is done.

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_cg.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index bd0763d6..6c7ad076 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -1432,11 +1432,12 @@ dt_cg_tstring_reset(dtrace_hdl_t *dtp)
 		ts = dtp->dt_tstrings;
 		for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
 			ts->offset = i * size;
+	} else {
+		/* Ensure that all allocated tstrings were freed correctly. */
+		ts = dtp->dt_tstrings;
+		for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
+			assert(ts->in_use == 0);
 	}
-
-	ts = dtp->dt_tstrings;
-	for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
-		ts->in_use = 0;
 }
 
 /*
-- 
2.45.2

Re: [PATCH 2/3] cg: validate tstring alloc/free

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
though apparently
undiscriminantly ->
indiscriminately

On 7/15/25 15:50, Kris Van Hees wrote:
> Rather than undiscriminantly resetting tstring allocations at the
> beginning of a compilation, actually verify that alloc/free of
> tstrings is done correctly, i.e. that none are left allocated after
> compilation is done.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   libdtrace/dt_cg.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index bd0763d6..6c7ad076 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1432,11 +1432,12 @@ dt_cg_tstring_reset(dtrace_hdl_t *dtp)
>   		ts = dtp->dt_tstrings;
>   		for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
>   			ts->offset = i * size;
> +	} else {
> +		/* Ensure that all allocated tstrings were freed correctly. */
> +		ts = dtp->dt_tstrings;
> +		for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
> +			assert(ts->in_use == 0);
>   	}
> -
> -	ts = dtp->dt_tstrings;
> -	for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
> -		ts->in_use = 0;
>   }
>   
>   /*

Re: [PATCH 2/3] cg: validate tstring alloc/free

[Eugene Loh]

I was a little concerned about this patch, but also wanted to expedite 
review.  FWIW, testing shows

test/unittest/builtinvar/tst.psinfo-bug21974606.d:
test/unittest/builtinvar/tst.psinfo-bug22561297.d:
test/unittest/builtinvar/tst.psinfo.d:
test/unittest/builtinvar/tst.psinfo1.d:
test/unittest/fbtprovider/tst.entryargs2.sh:
test/unittest/funcs/copyinstr/tst.copyinstr-high-maxsize.d:
test/unittest/funcs/copyinstr/tst.copyinstr-low-maxsize.d:
test/unittest/funcs/copyinstr/tst.copyinstr-no-maxsize.d:
test/unittest/funcs/copyinstr/tst.copyinstr.d:
test/unittest/proc/tst.exec.sh:
test/unittest/proc/tst.execfail.ENOENT.sh:
test/unittest/proc/tst.execfail.sh:
test/unittest/proc/tst.pr_psargs.d:
test/unittest/proc/tst.pr_psargs_other_task.d:
test/unittest/usdt/tst.argmap-typed-partial.d:
test/unittest/usdt/tst.argmap-typed.d:
test/unittest/variables/bvar/tst.execargs.d:
test/demo/spec/specopen.d:

test/unittest/dif/bcopy.d:

failing on every VM.  (Well, bcopy is listed separately since we skip it 
on ARM.)  I'll try to put together a patch, but let me know if you 
already knew about this and have a fix.

On 7/16/25 00:45, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> though apparently
> undiscriminantly ->
> indiscriminately
>
> On 7/15/25 15:50, Kris Van Hees wrote:
>> Rather than undiscriminantly resetting tstring allocations at the
>> beginning of a compilation, actually verify that alloc/free of
>> tstrings is done correctly, i.e. that none are left allocated after
>> compilation is done.
>>
>> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
>> ---
>>   libdtrace/dt_cg.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index bd0763d6..6c7ad076 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -1432,11 +1432,12 @@ dt_cg_tstring_reset(dtrace_hdl_t *dtp)
>>           ts = dtp->dt_tstrings;
>>           for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
>>               ts->offset = i * size;
>> +    } else {
>> +        /* Ensure that all allocated tstrings were freed correctly. */
>> +        ts = dtp->dt_tstrings;
>> +        for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
>> +            assert(ts->in_use == 0);
>>       }
>> -
>> -    ts = dtp->dt_tstrings;
>> -    for (i = 0; i < DT_TSTRING_SLOTS; i++, ts++)
>> -        ts->in_use = 0;
>>   }
>>     /*

Re: [PATCH 2/3] cg: validate tstring alloc/free

[Kris Van Hees]

On Wed, Jul 16, 2025 at 07:07:59PM -0400, Eugene Loh wrote:
> I was a little concerned about this patch, but also wanted to expedite
> review.  FWIW, testing shows
> 
> test/unittest/builtinvar/tst.psinfo-bug21974606.d:
> test/unittest/builtinvar/tst.psinfo-bug22561297.d:
> test/unittest/builtinvar/tst.psinfo.d:
> test/unittest/builtinvar/tst.psinfo1.d:
> test/unittest/fbtprovider/tst.entryargs2.sh:
> test/unittest/funcs/copyinstr/tst.copyinstr-high-maxsize.d:
> test/unittest/funcs/copyinstr/tst.copyinstr-low-maxsize.d:
> test/unittest/funcs/copyinstr/tst.copyinstr-no-maxsize.d:
> test/unittest/funcs/copyinstr/tst.copyinstr.d:
> test/unittest/proc/tst.exec.sh:
> test/unittest/proc/tst.execfail.ENOENT.sh:
> test/unittest/proc/tst.execfail.sh:
> test/unittest/proc/tst.pr_psargs.d:
> test/unittest/proc/tst.pr_psargs_other_task.d:
> test/unittest/usdt/tst.argmap-typed-partial.d:
> test/unittest/usdt/tst.argmap-typed.d:
> test/unittest/variables/bvar/tst.execargs.d:
> test/demo/spec/specopen.d:
> 
> test/unittest/dif/bcopy.d:
> 
> failing on every VM.  (Well, bcopy is listed separately since we skip it on
> ARM.)  I'll try to put together a patch, but let me know if you already knew
> about this and have a fix.

I am looking into it as well.  Clearly, this patch is catching issues as
expected.

Re: [PATCH 2/3] cg: validate tstring alloc/free

[Eugene Loh]

On 7/16/25 20:22, Kris Van Hees wrote:

> On Wed, Jul 16, 2025 at 07:07:59PM -0400, Eugene Loh wrote:
>> I was a little concerned about this patch, but also wanted to expedite
>> review.  FWIW, testing shows
>>
>> test/unittest/builtinvar/tst.psinfo-bug21974606.d:
>> test/unittest/builtinvar/tst.psinfo-bug22561297.d:
>> test/unittest/builtinvar/tst.psinfo.d:
>> test/unittest/builtinvar/tst.psinfo1.d:
>> test/unittest/fbtprovider/tst.entryargs2.sh:
>> test/unittest/funcs/copyinstr/tst.copyinstr-high-maxsize.d:
>> test/unittest/funcs/copyinstr/tst.copyinstr-low-maxsize.d:
>> test/unittest/funcs/copyinstr/tst.copyinstr-no-maxsize.d:
>> test/unittest/funcs/copyinstr/tst.copyinstr.d:
>> test/unittest/proc/tst.exec.sh:
>> test/unittest/proc/tst.execfail.ENOENT.sh:
>> test/unittest/proc/tst.execfail.sh:
>> test/unittest/proc/tst.pr_psargs.d:
>> test/unittest/proc/tst.pr_psargs_other_task.d:
>> test/unittest/usdt/tst.argmap-typed-partial.d:
>> test/unittest/usdt/tst.argmap-typed.d:
>> test/unittest/variables/bvar/tst.execargs.d:
>> test/demo/spec/specopen.d:
>>
>> test/unittest/dif/bcopy.d:
>>
>> failing on every VM.  (Well, bcopy is listed separately since we skip it on
>> ARM.)  I'll try to put together a patch, but let me know if you already knew
>> about this and have a fix.
> I am looking into it as well.  Clearly, this patch is catching issues as
> expected.

I guess, but those issues should be fixed before the patch is applied?  
I'll move onto something else unless you think it's a good idea for two 
of us to be looking at this at once.  I do not understand, though, why 
we check if dnp->dn_kind is DT_NODE_FUNC or DT_NODE_OP1 or DT_NODE_OP2 
or DT_NODE_OP3 or DT_NODE_DEXPR.

Re: [PATCH 2/3] cg: validate tstring alloc/free

[Kris Van Hees]

On Wed, Jul 16, 2025 at 08:45:19PM -0400, Eugene Loh wrote:
> On 7/16/25 20:22, Kris Van Hees wrote:
> 
> > On Wed, Jul 16, 2025 at 07:07:59PM -0400, Eugene Loh wrote:
> > > I was a little concerned about this patch, but also wanted to expedite
> > > review.  FWIW, testing shows
> > > 
> > > test/unittest/builtinvar/tst.psinfo-bug21974606.d:
> > > test/unittest/builtinvar/tst.psinfo-bug22561297.d:
> > > test/unittest/builtinvar/tst.psinfo.d:
> > > test/unittest/builtinvar/tst.psinfo1.d:
> > > test/unittest/fbtprovider/tst.entryargs2.sh:
> > > test/unittest/funcs/copyinstr/tst.copyinstr-high-maxsize.d:
> > > test/unittest/funcs/copyinstr/tst.copyinstr-low-maxsize.d:
> > > test/unittest/funcs/copyinstr/tst.copyinstr-no-maxsize.d:
> > > test/unittest/funcs/copyinstr/tst.copyinstr.d:
> > > test/unittest/proc/tst.exec.sh:
> > > test/unittest/proc/tst.execfail.ENOENT.sh:
> > > test/unittest/proc/tst.execfail.sh:
> > > test/unittest/proc/tst.pr_psargs.d:
> > > test/unittest/proc/tst.pr_psargs_other_task.d:
> > > test/unittest/usdt/tst.argmap-typed-partial.d:
> > > test/unittest/usdt/tst.argmap-typed.d:
> > > test/unittest/variables/bvar/tst.execargs.d:
> > > test/demo/spec/specopen.d:
> > > 
> > > test/unittest/dif/bcopy.d:
> > > 
> > > failing on every VM.  (Well, bcopy is listed separately since we skip it on
> > > ARM.)  I'll try to put together a patch, but let me know if you already knew
> > > about this and have a fix.
> > I am looking into it as well.  Clearly, this patch is catching issues as
> > expected.
> 
> I guess, but those issues should be fixed before the patch is applied?  I'll
> move onto something else unless you think it's a good idea for two of us to
> be looking at this at once.  I do not understand, though, why we check if
> dnp->dn_kind is DT_NODE_FUNC or DT_NODE_OP1 or DT_NODE_OP2 or DT_NODE_OP3 or
> DT_NODE_DEXPR.

They are being fixed before it is applied.  In fact, all but 1 are now fixed.
Patch comes tomorrow sometim after I am not overheating anymore.

Re: [PATCH 2/3] cg: validate tstring alloc/free

[Kris Van Hees]

On Wed, Jul 16, 2025 at 08:45:19PM -0400, Eugene Loh wrote:
> On 7/16/25 20:22, Kris Van Hees wrote:
> 
> > On Wed, Jul 16, 2025 at 07:07:59PM -0400, Eugene Loh wrote:
> > > I was a little concerned about this patch, but also wanted to expedite
> > > review.  FWIW, testing shows
> > > 
> > > test/unittest/builtinvar/tst.psinfo-bug21974606.d:
> > > test/unittest/builtinvar/tst.psinfo-bug22561297.d:
> > > test/unittest/builtinvar/tst.psinfo.d:
> > > test/unittest/builtinvar/tst.psinfo1.d:
> > > test/unittest/fbtprovider/tst.entryargs2.sh:
> > > test/unittest/funcs/copyinstr/tst.copyinstr-high-maxsize.d:
> > > test/unittest/funcs/copyinstr/tst.copyinstr-low-maxsize.d:
> > > test/unittest/funcs/copyinstr/tst.copyinstr-no-maxsize.d:
> > > test/unittest/funcs/copyinstr/tst.copyinstr.d:
> > > test/unittest/proc/tst.exec.sh:
> > > test/unittest/proc/tst.execfail.ENOENT.sh:
> > > test/unittest/proc/tst.execfail.sh:
> > > test/unittest/proc/tst.pr_psargs.d:
> > > test/unittest/proc/tst.pr_psargs_other_task.d:
> > > test/unittest/usdt/tst.argmap-typed-partial.d:
> > > test/unittest/usdt/tst.argmap-typed.d:
> > > test/unittest/variables/bvar/tst.execargs.d:
> > > test/demo/spec/specopen.d:
> > > 
> > > test/unittest/dif/bcopy.d:
> > > 
> > > failing on every VM.  (Well, bcopy is listed separately since we skip it on
> > > ARM.)  I'll try to put together a patch, but let me know if you already knew
> > > about this and have a fix.
> > I am looking into it as well.  Clearly, this patch is catching issues as
> > expected.
> 
> I guess, but those issues should be fixed before the patch is applied?  I'll
> move onto something else unless you think it's a good idea for two of us to
> be looking at this at once.  I do not understand, though, why we check if

See:
	[PATCH] tstring: fix leaks
just posted

> dnp->dn_kind is DT_NODE_FUNC or DT_NODE_OP1 or DT_NODE_OP2 or DT_NODE_OP3 or
> DT_NODE_DEXPR.

Because the way parser nodes are implemented, a rather complex union is used,
and various members in it are used by different node kinds for different
things.

Those nodes (and my patch adds one) are the nodes that can hold a tstring.