[DTrace-devel] [PATCH 1/2] usdt: is-enabled probe support
Nick Alcock
nick.alcock at oracle.com
Tue May 16 18:00:17 UTC 2023
On 13 May 2023, Kris Van Hees stated:
> On Wed, Apr 19, 2023 at 04:44:12PM +0100, Nick Alcock via DTrace-devel wrote:
>> This implementation is in many ways similar to v1's, but contains one
>> significant simplification: rather than is-enabled probes appearing to
>> the user as a function that returns a 1 or 0, they appear as a function
>> that returns void and takes the address of a local variable: the
>> _IS_ENABLED macros use the GCC statement-expression extension
>> to declare and initialize a distinct local for each use of the
>> is-enabled probe:
>>
>> ({ uint32_t enabled = 0; \
>> __dtraceenabled_foo___bar(&enabled); \
>> enabled; })
>>
>> The is-enabled trampoline writes a 1 into this local variable. This
>> allows the dt_link code to discard all the architecture-dependent code
>> dealing with return value handling and treat is-enabled probes just like
>> other USDT probes: it still tracks whether is-enabled probes are used,
>> but barely uses the info for anything but DOF-version validation.
>
> The last part of this paragraph is confusing. When you say "barely" it
> makes me wonder what it uses the info for aside from DOF validation.
Nothing, in the end (there was a bit more when I wrote that). Dropped
the barely and changed from 'DOF-version validation' to 'setting the DOF
version', since mostly what it does is forces the DOF version to v3 if
is-enabled probes are present. (It does also make sure there is no v2
DOF hanging around, now that the DOF version stuff precedes it.)
>> The downside of this approach, of course, is that users need the
>> statement-expression extension to work, and that old is-enabled probes
>> from DTrace v1 aren't going to work without regeneration with dtrace
>> -G. Neither of these seem likely to be serious problems in practice. (An
>> upcoming commit will prevent v1 is-enabled probes from being mistaken
>> for v2 probes.)
>
> This needs to change in iew of the v1 prevention patch going in before this
> one.
Yeah, adjusted.
>> We use #pragma system_header to suppress -pedantic warnings about
>> use of the extension.
>>
>> Internally, there are tiny changes in the dtprobed and dt_pid
>> uprobe-registration-and-creation layers to put is-enabled probes in a
>> different uprobe group to prevent their names clashing with USDT probes
>> with the same name (dt_pid_is_enabled/ rather than dt_pid/). Normal USDT
>> probes enable the correspondinng is-enabled probe, if any, when they are
>> enabled: those probes have custom trampoline that copy a 64-bit-wide 1
>> into the first function parameter (and have truncated paramter-copying
>> code that avoids preserving any parameter but the first, because there
>> will never be any more).
>>
> paramter -> parameter
>
>> Unanswered questions:
>> - is any of the stuff in dt_prov_usdt unnecessary? There's quite a lot
>> of boilerplate I am unsure of the need for.
>> - the is-enabled probes somehow get enabled even if I forget to
>> fix up the provider name to prvname_is_enabled in enable().
>> I have no idea how, but it makes me wonder if the special "USDT
>> enabling auto-enables the corresponding is-enabled probe" code is
>> actually needed.
>
> This is a bit of a problem. These issues should be resolved one way or another
> before this goes in.
So the somehow getting enabled must have been a result of my testing the
wrong tree or something: everything works (or, rather, doesn't work) as
I'd expect if I take the fixup out now.
So the only remaining bit is the boilerplate, which I guess we can clean
up later if it's unnecessary (I don't *think* it is, but you'd probably
know better than me).
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 14ae21f764f59..27ca56438afac 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -314,11 +314,13 @@ dt_cg_tramp_copy_regs(dt_pcb_t *pcb)
>> * function: if zero, they are laid out as at the call instruction, before the
>> * function is called (as is done for e.g. usdt).
>> *
>> + * if one_arg is set, copy only one arg.
>
> I don't think this is a good idea. Since this is a special case, you might
> as well just do the load/store conbination for arg0 in the trampoline, rather
> than changing this function for a very special case.
... it is only *two lines* isn't it. You're quite right: moved, dropped
this bit.
>> +/*
>> + * Copy the given immediate value into the address given by the specified probe
>> + * argument.
>> + */
>> +void
>> +dt_cg_tramp_copyout_val(dt_pcb_t *pcb, uint32_t val, int arg)
>> +{
>> + dt_regset_t *drp = pcb->pcb_regs;
>> + dt_irlist_t *dlp = &pcb->pcb_ir;
>> +
>> + emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_FP, DT_TRAMP_SP_SLOT(0), val));
>> +
>> + if (dt_regset_xalloc_args(drp) == -1)
>> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_7, DMST_ARG(arg)));
>> + emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_FP));
>> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DT_TRAMP_SP_SLOT(0)));
>> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, sizeof(uint32_t)));
>> + dt_regset_xalloc(drp, BPF_REG_0);
>> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_write_user));
>> +
>> + /* XXX any point error-checking here? What can we possibly do? */
>> + dt_regset_free(drp, BPF_REG_0);
>> + dt_regset_free_args(drp);
>> +}
>
> I'd rather see this be in the trampoline code of the provider. There is no
> other code using this so there is no need to have a generic function. And it
> is very much tied to the implementation details of is-enabled probes.
It's complex enough that I'd rather have it at least as a separate
function. I'm happy to move it into dt_prov_uprobe() -- that's where it
started life, before I thought that it seemed generic enough that maybe
it should liv in dt_cg so other people could use it too. (Its code is
very similar to a lot of other dt_cg stuff, and having it in the same
file encourages later refactoring.)
>> static void
>> dt_cg_spill_store(int reg)
>> {
>> diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
>> index 95bf507ff7fb1..320862682cb43 100644
>> --- a/libdtrace/dt_cg.h
>> +++ b/libdtrace/dt_cg.h
>> @@ -24,9 +24,10 @@ extern void dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act);
>> extern void dt_cg_tramp_prologue(dt_pcb_t *pcb);
>> extern void dt_cg_tramp_clear_regs(dt_pcb_t *pcb);
>> extern void dt_cg_tramp_copy_regs(dt_pcb_t *pcb);
>> -extern void dt_cg_tramp_copy_args_from_regs(dt_pcb_t *pcb, int called);
>> +extern void dt_cg_tramp_copy_args_from_regs(dt_pcb_t *pcb, int called, int one_arg);
>
> See above - should not be needed.
Definitely ack.
>> extern void dt_cg_tramp_copy_pc_from_regs(dt_pcb_t *pcb);
>> extern void dt_cg_tramp_copy_rval_from_regs(dt_pcb_t *pcb);
>> +extern void dt_cg_tramp_copyout_val(dt_pcb_t *pcb, uint32_t val, int arg);
>
> See above - should not be needed.
I'm wincing, but I moved it (and renamed it a bit since it is no longer
in dt_cg).
>> @@ -436,17 +432,18 @@ dt_header_probe(dt_idhash_t *dhp, dt_ident_t *idp, void *data)
>>
>> if (!infop->dthi_empty) {
>> if (fprintf(infop->dthi_out,
>> - "#if !defined(__aarch64__) && !defined(__sparc)\n"
>> + "#ifdef __GNUC__\n"
>> "#define\t%s_%s_ENABLED() \\\n"
>> - "\t__dtraceenabled_%s___%s()\n"
>> + "\t({ uint32_t enabled = 0; \\\n"
>> + "\t__dtraceenabled_%s___%s(&enabled); \\\n"
>> + "\t enabled; })\n"
>> "#else\n"
>> - "#define\t%s_%s_ENABLED() \\\n"
>> - "\t__dtraceenabled_%s___%s(0)\n"
>> + "#define\t%s_%s_ENABLED() (1)\\n"
>
> This seems to be a bit problematic. If the code is not compiled with GCC then
> we silently turn is-enabled probes into a true value?
I don't see any alternative. Turning them into a false value breaks
probing anything protected by such probes when non-statement-expression-
supporting compilers are not used, which is awful: leaving them
returning true should only cause a slowdown when not probing, and if it
does more than that then people are abusing is-enabled probes to do more
than guard expensive data collection for probing and their programs will
break whenever tracing is off (which is most of the time, so it's most
unlikely people will do that).
> We should preferably
> have an alternative that does not depend on GCC extension syntax, or worst case
I couldn't think of one that didn't have major downsides on any platform
so arcane or archaic it didn't support the statement-expression
extension. The problem is that variable definition...
The only possibilities I thought of appear to involve introducing whole
new *functions*, which is of course not something we can do inside a
macro (obviously they have to be non-nested functions since nested
functions are a genuinely obscure GCC extension). We could introduce a
static inline in the .d header and call that from the macro, but *that
too* is a GCC extension (and requires us to have a new function for
every .h file, with a varying name) -- and introducing static
non-inlines will add significant inefficiency in any compilers old
enough that they don't support statement expressions, since all such
that I am aware of *also* don't eliminate unused static functions, and
there'd be one of these for every is-enabled probe, unless you passed
the new function the probe as a function pointer, and now the passing-in
of that function pointer looks nothing like a normal probe call and you
need special code in dt_link for it... this is close to what I
originally thought up (with the function in drti.o) and you rightly said
that was unnecessarily overcomplicated.
I am sceptical that any compilers not supporting the
statement-expression extension are in use on Linux systems anyway. For
many releases you couldn't call glibc's open() or open64() without using
them (and there was no fallback at all, and nobody complained). That was
more than fifteen years ago, and statement-expression extension support
certainly hasn't got *rarer* since then. So I think they're not just
uncommon but extinct outside of very rare edge cases (like people
running unusual compilers like tinycc or mescc during very early
bootstrap as part of stage0/MES), and I don't think it's worth going to
any lengths at all to deal with such a theoretical concern.
... actually, even tinycc supports statement expressions! (as of 0.9.16,
which is so old there are no downloads for it on Savannah, so it
predates 2008). OK if even that supports this extension I think we can
assume that it's ubiquitous.
I just dug around because I'd like to find some of these mythical
monsters now. On
<https://community.intel.com/t5/Intel-C-Compiler/Statement-expr-inside-of-assume-incorrectly-considered-to-have/m-p/1182049>
there is a mention, as reliable as anything else found on the Internet
no doubt, that statement expressions are supported by
> icc back to at least version 9, PGI, Oracle Developer Studio 5.12+, XL
> C/C++ 11+, Cray since at least 8.1, tinycc 0.9.26+, armcc since at
> least 4.1
which is wider than I was expecting; and citations of non-supporting
compilers:
> MSVC, Pelles, Digital Mars C, and I *think* IAR but I'd have to check
> on that.
I've never even *heard* of half of those, and the rest are either very
non-Linux or incredibly obscure. I think we can just ignore the edge
case of compilers not supporting statement expressions after we've done
the minimal necessary to get the code correct-but-not-fast, and turning
is-enabled into if (1) does that.
Aside: what you *cannot* safely do is use statement expressions in
initializers at global scope (it breaks in C++). But nobody will ever do
that with is-enabled probes since you can't put a conditional in an
initializer.
Also you can expect disaster doing things like jumping out of statement
expressions that contain non-POD types, let alone into them, but we
don't do anything like that and the user can't do it either unless they
hand-hack the generated provider headers (in which case they get to keep
all the pieces).
> at least issue a warning that we do not support is-enabled probes on whatever
> compiler people are using.
I tried, but there is no way to portably emit warnings :( #warning is...
a GCC extension! It's almost ubiquitous, so maybe we could do that, but
where can we put the #warning?
We can't put it in the is-enabled macro definition because you can't put
#warning or similar things inside macros; we can't put it in the header
or you'd get the warning on affected compilers even in the much more
common case that you weren't using is-enabled probes at all; and we
can't emit it inline because you can't put #warning inside conditionals
(not on any compiler I'm aware of, anyway -- and the only way we could
even emit it there would be to put it in the macro definition, which we
can't do, see above).
There is of course an _Pragma that emits warnings that doesn't suffer
from some of these problems, but it's called, uh, "_Pragma GCC warning"
and thus is quite unlikely to exist on non-GCC compilers so limited that
they don't support the statement-expression extension :) given that
_Pragma is about twenty years younger than said extension and a lot of
current compilers don't support it even now...
(I did a bit more digging. #warning support seems to be *rarer* than
support for the GCC statement expression extension! Cray's --
necessarily prehistoric -- compiler at least doesn't support it, at
least judging by various programs that have ifdefs to avoid #warning on
Cray. Of course, given that even the X.org client headers dropped
support for the Cray compiler years ago I don't think we need to care
much.)
>> @@ -175,7 +175,7 @@ static void trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>> dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
>> emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
>> } else
>> - dt_cg_tramp_copy_args_from_regs(pcb, 1);
>> + dt_cg_tramp_copy_args_from_regs(pcb, 1, 0);
>
> See above - not needed.
Ack, dropped.
>> @@ -310,7 +330,7 @@ static int provide_usdt_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp)
>> return provide_probe(dtp, psp, psp->pps_prb, &dt_usdt, PP_IS_FUNCALL);
>> }
>>
>> -static void enable(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> +static void enable(dtrace_hdl_t *dtp, dt_probe_t *prp, int is_enabled)
>
> This seems a bit confusing to me. THe is_enabled argument seems to indicate
> whether we're trying to enable an is-enabled probe but if I read this correctly
> it actually enabled both the USDT probe and its related is-enabled probes.
> Perhaps is_usdt might be a better name? Or can we determine the type of probe
> based on the passed in prp (e.g. look at what the provider is)?
I meant is_enabled to mean 'enable any is-enabled probes that might
exist', and enable_is_enabled is a horrible name. You're right that when
called with is_enabled == 1, it's being called for the *USDT* probe and
has to enable *both*.
So is_usdt is a much better name, switched. :)
>> @@ -333,6 +367,19 @@ static void enable(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> dt_list_append(&dtp->dt_enablings, prp);
>> }
>>
>> +static void enable_plain(dtrace_hdl_t *dtp, dt_probe_t *prp)
>
> Surely enable_pid would be better?
Yes 'plain' is awful. Adjusted.
>> else
>> dt_cg_tramp_copy_args_from_regs(pcb,
>> - !(upp->flags & PP_IS_FUNCALL));
>> + !(upp->flags & PP_IS_FUNCALL),
>> + 0);
>
> See above - not needed.
Ack.
I also adjusted the comment above trampoline() to read
* Generate a BPF trampoline for a pid or USDT probe.
*
* The trampoline function is called when one of these probes triggers, and it
* must satisfy the following prototype:
*
* int dt_uprobe(dt_pt_regs *regs)
*
since it hasn't been just for pid probes for a while, except insofar as
usdt probes are a kind of pid probe.
>> + dt_cg_tramp_copy_regs(pcb);
>> + dt_cg_tramp_copy_args_from_regs(pcb,
>> + !(upp->flags & PP_IS_FUNCALL), 1);
>
> See above - just inline the copying of the one argument.
Replaced with
/*
* Copy in the first function argument, a pointer value to which
* the is-enabled state of the probe will be written (necessarily
* 1 if this probe is running at all).
*/
emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, PT_REGS_ARG0));
emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
which is just terrible code duplication, I'm sure you'll agree.
>> + /*
>> + * Generate a composite conditional clause, as above, except that rather
>> + * than emitting call_clauses, we emit copyouts instead, using
>> + * dt_cg_copyout_reg().
>> + *
>> + * if (pid == PID1) {
>> + * *arg0 = 1;
>> + * goto exit;
>> + * } else if (pid == PID2) {
>> + * *arg0 = 1;
>> + * goto exit;
>> + * } else if (pid == ...) {
>> + * < ... >
>> + * }
>
> Can't this be rendered in code where the *arg0 = 1 occurs just once, and any
> matching case jumps to that code?
Yes. I didn't think of it. Rejigged, it's quite a lot simpler now: let's
see if it actually works. (It should...)
>> + /*
>> + * Check whether this pid-provider probe serves the current
>> + * process, and copy out a 1 into arg 0 if so.
>> + */
>> + emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, pid, lbl_next));
>> + dt_cg_tramp_copyout_val(pcb, 1, 0);
>
> Just inline it.
Moved into a function right above trampoline_is_enabled() because I
found that easier to read. It's not in dt_cg any more anyway. (Even
after the rejig above, it's still clearer to leave it factored out:
among other things, leaving the argument-to-write and the like as
named parameters rather than literal 0's serves as crucial documentation
for me at least to remember what on earth it's doing.)
--
NULL && (void)
More information about the DTrace-devel
mailing list