[DTrace-devel] [PATCH] ERROR probe implementation

[Kris Van Hees]

On Thu, Jan 21, 2021 at 12:53:49PM -0500, Eugene Loh wrote:
> On 1/21/21 12:46 PM, Kris Van Hees wrote:
> 
> > On Thu, Jan 21, 2021 at 12:25:56PM -0500, Eugene Loh wrote:
> >> On 1/21/21 2:04 AM, Kris Van Hees wrote:
> >>
> >>> On Wed, Jan 20, 2021 at 11:23:59PM -0500, Eugene Loh wrote:
> >>>> 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 problem is that they both implement the same code at their beginning, then
> >>> one implements extra code, and then they again share code.  So I cannot just
> >>> call one from the other.
> >> Right, but the amount of code they're sharing has grown. Meanwhile,
> >> conditionalizing the "extra code" that's in the middle is easy enough.
> >> There are different ways of doing this, but if one is okay with using
> >> "if (act != DT_ACTIVITY_ACTIVE)" then not only is dt_cg_tramp_epilogue()
> >> just a call to dt_cg_tramp_epilogue_advance(), but the "activity" code
> >> in dt_prov_dtrace.c's trampoline() function could get cleaned up a
> >> little as well.
> > Hm, yes, but I really prefer the providers to be able to just call
> > dt_cg_tramp_epilogue() because dt_cg_tramp_epilogue_advance() is a special
> > case that only exists because of the BEGIN and END probes.
> 
> Sorry, I didn't mean that the other providers needed any changes.  I 
> meant only:
> - dt_cg_tramp_epilogue() could be implemented as a one-liner
> - the dtrace provider's trampoline could be simplified vis-a-vis "act" 
> stuff

Woops, my bad - I misunderstood.

> >>>> 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
> >>> Well, it makes it possible to write "return error(...)" which is similar to
> >>> the typical "return dt_probe_error(...)" that one would use.  To make that
> >>> possible we need a macro expression using the ({ ... }) syntax.  That is not
> >>> the same as a multi-statement code block.
> >> My point on ({...}) was that if we are willing to use this construct
> >> (which is available in "newer" gcc compilers, I thought), then our
> >> advice in CODING-STYLES regarding multiple-statement macros should be
> >> updated.  I can update CODING-STYLE (and our use of "do {} while(0)") in
> >> a separate patch if you like.
> > Sure.  I see no reason to not use that construct, though I do think it should
> > be used sparingly.
> Okay.  Why sparingly?

Well, I wouldn't put that into the coding style document as a comment, but I
find that the ({ ... }) construct is not always as clear to people, perhaps
because it is not widely used.  It can also lead to inadvertedly introducing
side effects in e.g. conditionals by using a macro that is valid as an
expression but actually does a whole lot more.

Anyway, I don't think the coding style needs to address that - we can handle
that in the code review process.