[DTrace-devel] [PATCH 04/12] options: report EDT_NOREG for iregs < DIF_DIR_NREGS

[Eugene Loh]

Hardwiring the use of %r7 and %r8 in trampoline cg has presumably 
nothing to do with the number of DIF iregs -- that is, with our ability 
to handle expressions like:

     a + b * (c + d * (e + f * (g + h * (i + j * (k + l)))))

For example, I think the 3/12 patch would work just fine with something 
like:

     if (drp->dr_size > 7)
         dt_regset_xalloc(drp, BPF_REG_7);
     if (drp->dr_size > 8)
         dt_regset_xalloc(drp, BPF_REG_8);

(plus similar regset_free code).  That is, if regset manages %r7 or %r8, 
tell regset we are using them.  If not, do not worry since regset will 
not try to use them anyhow.  In short, patch 3/12 does not need -xiregs 
changes.

Now, if you simply want to deprecate -xiregs:

1)  The EDT_NOREG check in dt_opt_iregs() does not make sense to me.  
How about,

      dt_opt_iregs(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
      {
             int n;

     -       if (arg == NULL || (n = atoi(arg)) <= 0 || n > DIF_DIR_NREGS)
     +       if (arg == NULL || (n = atoi(arg)) <= 0 || n != DIF_DIR_NREGS)
                     return dt_set_errno(dtp, EDT_BADOPTVAL);

             dtp->dt_conf.dtc_difintregs = n;
             return 0;
      }

That is simpler and more direct than the EDT_NOREG error. Afterall, if 
your entire D program is 'BEGIN {exit(0)}', then asking -xiregs=7 is not 
a matter of too few DIF integer registers.  It is a matter than we have 
hardwired our code so that -xiregs needs a particular value.

2)  The tst.iregs.sh test no longer has anything to do with the 
complexity of DIF expressions.  It is simply checking whether a script 
is trying to change -xiregs.  As such, the following is simpler and more 
direct:

     for n in 10 9 8 7 6; do
         $dtrace -xiregs=$n -qn 'BEGIN { exit(0); }'
         echo "iregs $n gives $?"
     done

with the corresponding changes in the .r file.  If we want testing on 
some actual DIF reg spill/fill code, we can augment 
test/unittest/codegen/tst.kernel_read_scalar_regspill.d

3)  The preceding (3/12) patch in its current form requires guardrails 
on -xiregs, since we otherwise hit assertion failures when trying to 
generate trampoline code.  So, if patch 3/12 requires the hardwired 
value of -xiregs, then the dt_options.c check on -xiregs should be part 
of that patch.

On 1/5/24 20:11, Kris Van Hees wrote:
> On Fri, Jan 05, 2024 at 04:22:09PM -0500, Eugene Loh via DTrace-devel wrote:
>> See my comments to the 03/12 patch.
> As you say in the comments there, -xiregs has become effectively obsolete,
> but since it is an option DTrace accepts, we probably should continue to
> accept it.  This patch ensure sthat the only valid value is DIF_DIR_NREGS.
>
> As some point we can and shouold change this to be the number of BPF regs
> that are available for general use.  But even then, we cannot really support
> lowering that limit because there are so few BPF registers we can really use
> anyway due to the function call clobberingh of %r0-%r5.
>
> But this patch is the minimal change needed to ensure dtrace continues to
> work.
>
>> On 1/5/24 00:26, Kris Van Hees via DTrace-devel wrote:
>>> The trampoline generation code hardcodes the use of %r7 and %r8, which
>>> means that iregs cannot be less than 8 (DIF_DIR_NREGS).
>>>
>>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>>> ---
>>>    libdtrace/dt_options.c            |  4 +++-
>>>    test/unittest/options/tst.iregs.r | 26 +++++++-------------------
>>>    2 files changed, 10 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_options.c b/libdtrace/dt_options.c
>>> index 6f7cf9e2..18b8f43a 100644
>>> --- a/libdtrace/dt_options.c
>>> +++ b/libdtrace/dt_options.c
>>> @@ -1,6 +1,6 @@
>>>    /*
>>>     * Oracle Linux DTrace.
>>> - * Copyright (c) 2007, 2023, Oracle and/or its affiliates. All rights reserved.
>>> + * Copyright (c) 2007, 2024, Oracle and/or its affiliates. All rights reserved.
>>>     * Licensed under the Universal Permissive License v 1.0 as shown at
>>>     * http://oss.oracle.com/licenses/upl.
>>>     */
>>> @@ -266,6 +266,8 @@ dt_opt_iregs(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
>>>    	if (arg == NULL || (n = atoi(arg)) <= 0 || n > DIF_DIR_NREGS)
>>>    		return dt_set_errno(dtp, EDT_BADOPTVAL);
>>> +	if (n < DIF_DIR_NREGS)
>>> +		return dt_set_errno(dtp, EDT_NOREG);
>>>    	dtp->dt_conf.dtc_difintregs = n;
>>>    	return 0;
>>> diff --git a/test/unittest/options/tst.iregs.r b/test/unittest/options/tst.iregs.r
>>> index 4926b28d..f67e7b07 100644
>>> --- a/test/unittest/options/tst.iregs.r
>>> +++ b/test/unittest/options/tst.iregs.r
>>> @@ -1,25 +1,13 @@
>>>    7
>>>    iregs 8 gives 0
>>> -7
>>> -iregs 7 gives 0
>>> -7
>>> -iregs 6 gives 0
>>> +iregs 7 gives 1
>>> +iregs 6 gives 1
>>>    iregs 5 gives 1
>>>    iregs 4 gives 1
>>>    iregs 3 gives 1
>>>    -- @@stderr --
>>> -dtrace: invalid probe specifier BEGIN {
>>> -        a = b = c = d = e = f = g = h = i = j = k = l = 1;
>>> -        trace(a + b * (c + d * (e + f * (g + h * (i + j * (k + l))))));
>>> -        exit(0);
>>> -    }: Insufficient registers to generate code
>>> -dtrace: invalid probe specifier BEGIN {
>>> -        a = b = c = d = e = f = g = h = i = j = k = l = 1;
>>> -        trace(a + b * (c + d * (e + f * (g + h * (i + j * (k + l))))));
>>> -        exit(0);
>>> -    }: Insufficient registers to generate code
>>> -dtrace: invalid probe specifier BEGIN {
>>> -        a = b = c = d = e = f = g = h = i = j = k = l = 1;
>>> -        trace(a + b * (c + d * (e + f * (g + h * (i + j * (k + l))))));
>>> -        exit(0);
>>> -    }: Insufficient registers to generate code
>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel