[DTrace-devel] [PATCH] Fix size of string data in the trace output buffer
Kris Van Hees
kris.van.hees at oracle.com
Tue Sep 7 23:30:36 PDT 2021
On Wed, Sep 08, 2021 at 12:49:12AM -0400, Eugene Loh wrote:
> I guess this is okay, particularly since we'll probably be revisiting
> all this soon anyhow if/when the length prefix is removed. So:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> but questions/comments below.
>
> To start with, isn't there a common case where the source is already a
> "limited" string, in which case one can just do a single straight copy,
> length prefix and all?
Sure, but the overhead of doing it withouot optimizing for that case isn't
much at all. And optimization can definitely be done later if it is deemed
necessary.
> Also, does dt_cg_store_var() have the same issues?
No, this is only really needed for store_val because of how the consumer
handles strings.
> On 9/7/21 8:37 PM, Kris Van Hees wrote:
> > The size of string data items in the trace output buffer was not taking
> > into account that we use a 2-byte length prefix and it also did not
> > include the terminating NUL byte.
> >
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_cg.c | 27 +++++++++++++++--------
> > test/unittest/codegen/tst.str_data_size.d | 21 ++++++++++++++++++
> > test/unittest/codegen/tst.str_data_size.r | 4 ++++
> > 3 files changed, 43 insertions(+), 9 deletions(-)
> > create mode 100644 test/unittest/codegen/tst.str_data_size.d
> > create mode 100644 test/unittest/codegen/tst.str_data_size.r
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index a8756581..acb7c6fa 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -939,39 +939,48 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> > int reg = dt_regset_alloc(drp);
> >
> > off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, kind,
> > - size, 1, pfp, arg);
> > + size + DT_STRLEN_BYTES + 1, 1, pfp, arg);
> >
> > /*
> > * Retrieve the length of the string, limit it to the maximum
> > - * string size, and adjust for the terminating NUL byte and the
> > - * length prefix.
> > + * string size, and store it in the buffer at [%r9 + off].
> > */
> > dt_cg_strlen(dlp, drp, reg, dnp->dn_reg);
> > emit(dlp, BPF_BRANCH_IMM(BPF_JLT, reg, size, size_ok));
>
> Okay, though LT->LE probably makes more sense.
True. Fixed.
> > emit(dlp, BPF_MOV_IMM(reg, size));
> > emitl(dlp, size_ok,
> > - BPF_ALU64_IMM(BPF_ADD, reg, 1 + DT_STRLEN_BYTES));
> > + BPF_MOV_REG(BPF_REG_0, reg));
> > + emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 8));
> > + emit(dlp, BPF_STORE(BPF_B, BPF_REG_9, off, BPF_REG_0));
> > + emit(dlp, BPF_STORE(BPF_B, BPF_REG_9, off + 1, reg));
> >
> > /*
> > - * Copy string data to the output buffer at [%r9 + off]. The
> > - * amount of bytes copied is the lesser of the data size and
> > - * the maximum string size.
> > + * Copy the string to the output buffer at
> > + * [%r9 + off + DT_STRLEN_BYTES] because we need to skip the
> > + * length prefix.
> > */
> > if (dt_regset_xalloc_args(drp) == -1)
> > longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >
> > emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off + DT_STRLEN_BYTES));
> > emit(dlp, BPF_MOV_REG(BPF_REG_2, reg));
> > dt_regset_free(drp, reg);
> > emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> > dt_regset_free(drp, dnp->dn_reg);
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, DT_STRLEN_BYTES));
> > if (dnp->dn_tstring)
> > dt_cg_tstring_free(pcb, dnp);
>
> How about adding the +DT_STRLEN_BYTES instruction before the
> regset_free(dnp->dn_reg)? That way, instruction generation (for the two
> %r3 instructions) is consolidated and code-generator resource management
> (reg and tstring) is consolidated.
Actually, moving that instruction below the dt_cg_tstring_free() makes even
more sense because after the 'mov %r3, dnp->dn_reg' we no longer need the
dnp->dn_reg.
> > dt_regset_xalloc(drp, BPF_REG_0);
> > emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> > dt_regset_free_args(drp);
> > - dt_regset_free(drp, BPF_REG_0);
> > +
> > + /*
> > + * Write the NUL terminating byte in case we are truncating.
> > + */
> > + emit(dlp, BPF_MOV_REG(BPF_REG_0, BPF_REG_9));
> > + emit(dlp, BPF_ALU64_REG(BPF_ADD, BPF_REG_0, reg));
> > + emit(dlp, BPF_STORE_IMM(BPF_B, BPF_REG_0, off + DT_STRLEN_BYTES, 0));
>
> I sure do not understand the register management here. Earlier, you use
> %r0 for writing the length prefix, but never xalloc'ing the register.
> Here, you xalloc(r0) but you removed the free(r0). Isn't that wrong?
> But I tried iterating on this and it appears someone else is freeing the
> register? Who? I'm confused.
Ah, I forgot to paste the freeing of %r0 apparently - good catch.
>
> >
> > return 0;
> > }
> > diff --git a/test/unittest/codegen/tst.str_data_size.d b/test/unittest/codegen/tst.str_data_size.d
> > new file mode 100644
> > index 00000000..a928e829
> > --- /dev/null
> > +++ b/test/unittest/codegen/tst.str_data_size.d
> > @@ -0,0 +1,21 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +#pragma D option rawbytes
> > +#pragma D option strsize=5
> > +#pragma D option quiet
> > +
> > +BEGIN
> > +{
> > + trace(probeprov);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/codegen/tst.str_data_size.r b/test/unittest/codegen/tst.str_data_size.r
> > new file mode 100644
> > index 00000000..60483d74
> > --- /dev/null
> > +++ b/test/unittest/codegen/tst.str_data_size.r
> > @@ -0,0 +1,4 @@
> > +
> > + 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> > + 0: 00 05 64 74 72 61 63 00 ..dtrac.
> > +
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list