[DTrace-devel] [PATCH 3/4] ERROR probe implementation

[Eugene Loh]

I do not understand this code yet, but I figured partial feedback sooner 
would be more helpful than more feedback later.

libdtrace/dt_pcb.h
   why was the copyright year updated?

Need a comment about why the brand-new test 
tst.clause_scope-begin-ended.d is xfail.  Apparently, the patch is a WIP?

Typo in dt_bpf_reloc_error_prog():
   - We need to turns "call dt_error" into a NOP.
   + We need to turn "call dt_error" into a NOP.

Results file tst.clause_scope-regular.r expects tick-10ms to be ID 
72958, which will not generally be true.  The test should not assume 
particular IDs for profile::: probes.

In dt_cc.c, you introduce no_deps, which is related to DT_IDFLG_CGREG.  
It might be easier to follow the logic if whatever is going on is 
tracked with fewer names.  That is, if we're checking for CGREG, then a 
name that suggests CGREG might be simpler than introducing a new name 
like no_deps.  I admit, though, CGREG is not the most suggestive name to 
start with. Ick.  :^(

Should #if 0 dt_errprog-related code be removed?

In bpf/probe_error.c, you say arg0==NULL, but we do not document any 
arg0 for ERROR.  (Also, in that block comment, FWIW, you use spaces and 
tabs inconsistently.)

I'm confused by the comment block in front of dt_cg_probe_error().  It says,
     Generate code for a fault condition.  A call is made to 
dt_probe_error() to
     set the fault information, and then a non-zero return from the 
function is
     performed.
What does "non-zero return is performed" mean?  I assume this is 
related: the (redacted) code has
     dt_regset_xalloc_args(drp);
     BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX)
     BPF_MOV_IMM(BPF_REG_2, off)
     BPF_MOV_IMM(BPF_REG_3, fault)
     BPF_MOV_IMM(BPF_REG_4, illval)
     dt_regset_xalloc(drp, BPF_REG_0);   // spill %r0???
     BPF_CALL_FUNC("dt_probe_error")     // %r0 gets return value
     dt_regset_free_args(drp);
     dt_regset_free(drp, BPF_REG_0);     //  fill %r0???
     BPF_RETURN()
That is, when we BPF_RETURN(), the return value %r0 is either that from 
dt_probe_error() or else from before the call, depending on whether %r0 
had to be spilled and filled.  Do I understand that correctly?  I'm 
confused.  And the comment inside dt_cg_probe_error() says "return 
dt_probe_error()" while in bpf/probe_error.*, dt_probe_error() is void.  
Again, I'm confused.

I do not understand why you add a "prp" argument to 
dt_cg_tramp_call_clauses().

In the comments before dt_cg_epilogue(), you make this change:
     - *    4. If a fault was flagged, return 0.
     + *    4. If a fault was flagged, return the fault code.
But the testing of the fault code has been ripped out entirely. Why is 
this comment there?

It appears that adv_act is no longer needed in the dtrace provider's 
trampoline() function.  So, that code can be simplified.

You introduce a DT_CLSFLAG_ERROR, but I do not see it used anywhere.

Minor point, but in dt_bpf_load_progs(), you have the comment block

     First construct the ERROR probe program (to be included in probe
     programs that may trigger a fault).

     After constructing the program, we need to patch up any calls to
     dt_error because DTrace cannot handle faults in ERROR itself.
which is then followed by what feels like two code blocks (due to the 
blank line.  How about splitting the comment block into two, each 
sentence paired with its corresponding code.  E.g.,
     /*
      * Construct the ERROR probe program (to be included in probe
      * programs that may trigger a fault).  Mark it as inline-ready.
      */
     dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
     if (dp == NULL)
         return -1;
     idp->di_flags |= DT_IDFLG_CGREG;

     /*
      * Patch up any calls to dt_error because DTrace cannot
      * handle faults in ERROR itself.
      */
     dt_bpf_reloc_error_prog(dtp, dp);