[DTrace-devel] [PATCH] ERROR probe implementation

[Eugene Loh]

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

>>>> 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?