[DTrace-devel] [PATCH] ERROR probe implementation

[Eugene Loh]

I'm still wading through this, but I figured I'd send what I have.

I cannot tell if the following files need 2021 copyright notices or if 
this patch will be applied after some other patch that already updates 
those files.
     include/dtrace/difo_defines.h
     libdtrace/dt_as.c
     libdtrace/dt_cc.c
     libdtrace/dt_cg.c
     libdtrace/dt_consume.c
     libdtrace/dt_impl.h
     libdtrace/dt_prov_dtrace.c
     libdtrace/dtrace.h

The test .r results files are missing "at DIF offset" clauses.  Is this 
a loss of functionality?  The commit message explicitly mentions offset 
as being something we can record.

In dt_work.c, is "#if 0" supposed to be a "#ifdef FIXME" or dead code to 
be removed?   Same question with regards to other dt_errprog references.

Same question in dt_cg_epilogue().  In any case, the #if should include 
the block comment that describes the code;  if the code is dead, then so 
are the comments.

The function dt_cg_call_clause() has the typo "furthe".

Since both dt_cg_tramp_epilogue() and dt_cg_tramp_epilogue_advance() 
have grown -- and in exactly the same way -- perhaps it makes sense to 
implement the former by calling the latter.  This could also simplify 
dt_prov_dtrace.c, the only caller of dt_cg_tramp_epilogue_advance().

The two fprintf(stderr, ...) in dt_handle_err() look like debugging code 
that was not supposed to remain in the patch.

In bpf/get_bvar.c, the "#define error()" strikes me as funny.  To me, it 
does not feel like it buys much.  Anyhow, that's an opinion thing.  
FWIW, if we are okay with the ({...}) construct, then the following 
advice in CODING-STYLE should be updated:
         Macros with multiple statements should
         be enclosed in a do-while(0) block

In dt_difo_copy():

   x You have multiple memory allocations.  If any of them fail, you go
     to no_mem, which simply returns a NULL pointer.  I suppose that's
     okay, but it's very much unlike related code paths, which diligently
     free any successful allocations.  Wouldn't consistency dictate cleaning
     up any allocations in the event that not all allocations are 
successful?

   x The bulk of the function could be written much more compactly
     and readably something like this:

         #define DIFO_COPY_FLD(LEN, FLD, TYP) \
             if (odp->LEN > 0) { \
                 dp->LEN = odp->LEN; \
                 dp->FLD = dt_calloc(dtp, dp->LEN, sizeof(TYP)); \
                 if (dp->FLD == NULL) \
                     goto no_mem; \
                 memcpy(dp->FLD, odp->FLD, dp->LEN * sizeof(TYP)); \
             }

         DIFO_COPY_FLD(dtdo_len, dtdo_buf, struct bpf_insn)
         DIFO_COPY_FLD(dtdo_strlen, dtdo_strtab, char)
         DIFO_COPY_FLD(dtdo_varlen, dtdo_vartab, dtrace_difv_t)
         DIFO_COPY_FLD(dtdo_brelen, dtdo_breltab, dof_relodesc_t)
         DIFO_COPY_FLD(dtdo_krelen, dtdo_kreltab, dof_relodesc_t)
         DIFO_COPY_FLD(dtdo_urelen, dtdo_ureltab, dof_relodesc_t)

In bpf/Build, why isn't probe_error.c inserted in alphabetical order?

In dt_probe_error_clause():

   x two variables "name" are declared and used.
     How about renaming the inner one (or just inlining its single use and
     doing away with that variable altogether?

   x In dt_probe_error_clause(), IMHO, the loop body would be clearer
     like this:
         if (ridp != NULL && ...) {
             ... assign val ...
         }
         if (val > 0) {
             ... do relocation ...
         } else {
             ... copy relocation ...
         }
     I realize that's an opinion thing, but if the code is going to be 
left as
     is, at least perhaps the val declaration can be moved into the 
innermost
     block that uses it.

   x Why "10"?  (I think I know, but a comment would be nice.)

Why was dt_cg_node()'s DT_TOK_PTR/DT_TOK_DOT case handling a NULL 
pointer specially, and why has this handling been changed?  I realize 
that NULL pointers are bad addresses, but they are certainly not the 
only ones.  This is only one very special case.

Somewhat related to the above, why are tests changed from
     -    *(char *)NULL;
     +    trace(((struct task_struct *)NULL)->pid);
I guess part of the issue is that some of these constructs will be 
caught by the BPF verifier.