[DTrace-devel] [PATCH 01/12] Support loading scalars from kernel addresses

[Eugene Loh]

On 7/25/22 15:53, Kris Van Hees wrote:

> On Thu, Jul 21, 2022 at 08:24:48PM -0700, Eugene Loh via DTrace-devel wrote:
>> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>>
>> For other new tests, other sanity checks?  E.g., that max_pfn is a multiple
>> of 4096?  More stringent tests would be nice.
> No, because that is not relevant to DTrace.  Yes, it should be, but that is an
> aspect of what that variable means at the kernel level.

The point is not to check the kernel or a specific variable.  The point 
would be to design a test where we check the test's (that is, DTrace's) 
output.  If max_pfn is not suitable for that purpose, some other test 
should be designed.

>
>> A few other things.
>>
>> On 7/13/22 12:17, Kris Van Hees via DTrace-devel wrote:
>>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>>>
>>> +		  dt_regset_t *drp)
>>> +{
>>> +	if (dt_regset_xalloc_args(drp) == -1)
>>> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>>> +	emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_SP));
>>> +	emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
>>> +	emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
>> It is conceivable that dnp->dn_reg is either %r1 or %r2, in which case we
>> just finished overwriting that register.  Just make the %r3=%dnp->dn->reg
>> instruction the first of the three.
> This is the exiting issue with (lack of) proper register spilling.  In general,
> our code generation fails miserably whenever we start using %r1-%r5 and yet
> have to call a function (incl. helpers).  So while theoretically, the scenario
> you suggest could happen, it generally means we're already going to get into
> trouble.

The fix for this specific problem is simple.  The fix for the general 
problem is not (so far as I know) scheduled.  It would seem to me to be 
best to deal with this specific problem now.

> Also, if dnp->dn_reg is %r1 ir %r2, then it is very likely some other
> value is stored in %r3 (since we allocate registers in descending order) and
> will get clobbered.

I do not get this point.  If some other value is stored in %r3, it will 
be spilled/filled.

>
>>> +	emit(dlp, BPF_MOV_REG(dnp->dn_reg, BPF_REG_1));
>> It is conceivable that dnp->dn_reg is in the range %r1-%r5.  If that's the
>> case, then free_args() will fill the register with an old value and the
>> stack pointer will have been lost.  Might as well just omit this instruction
>> and regenerate the stack pointer when you need it.
> Same as above - I'd rather not optimmize based on the (broken) spill/fill
> code.

Yes, "same as above."  The solution to the immediate problem is simple 
and the solution to the general problem is (IIUC) nowhere in sight?