[DTrace-devel] [PATCH v2] cg: fix store/load for struct and union members for alloca pointers
Kris Van Hees
kris.van.hees at oracle.com
Thu Aug 31 20:39:55 UTC 2023
On Thu, Aug 31, 2023 at 04:30:38PM -0400, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Thanks.
> There should be .r files for the tests.
Not really, because these tests are exercising whether valid code is
generated. Other (already existing tests) exercise that stores and load
to and from alloca'd memory gets the correct data.
> There is no point in separate load and store tests. Specifically, the load
> tests are forced to include the store tests. Adding the "store" subsets is
> just more tests to maintain, understand, and run, with no better test
> coverage and virtually no extra diagnosibility. Just keep the load tests
> and simplify the test names.
Given that they are two distinct code paths and it is a pest to look at the
log for a failing test to deteermine whether it was the store or the load that
failed, having both really offers a benefit. Trust me on this - I have done
a lot of that while developing this bug fix patch. Yes, there is duplication
because it is best to store data before you try to load it, but having the
separate store tests helps identify problems more directly. And the cost is
truly minimal.
> It turns out, these two calls:
> dt_cg_alloca_access_check(dlp, drp, dreg, DT_ISIMM, size);
> dt_cg_alloca_ptr(dlp, drp, dreg, dreg);
> are basically always made in tandem... no surprise. (There is a little
> nuance to this claim, but the oddities are very minor.) It makes sense to
> combine them into one call, simplifying everywhere these calls are made.
The nuances are the reason I split them up.
> On 8/31/23 12:47, Kris Van Hees via DTrace-devel wrote:
> > Storing to and loading from struct or union members of alloca pointers
> > was not properly turning the alloca pointer into a real pointer, nor was
> > it performing boundary checks.
> >
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_cg.c | 32 ++++++++++-----
> > .../funcs/alloca/tst.alloca-array-load.d | 29 ++++++++++++++
> > .../funcs/alloca/tst.alloca-array-store.d | 28 +++++++++++++
> > .../funcs/alloca/tst.alloca-deref-load.d | 29 ++++++++++++++
> > .../funcs/alloca/tst.alloca-deref-store.d | 28 +++++++++++++
> > .../funcs/alloca/tst.alloca-struct-load.d | 33 +++++++++++++++
> > .../funcs/alloca/tst.alloca-struct-store.d | 32 +++++++++++++++
> > .../alloca/tst.alloca-struct-union-load.d | 40 +++++++++++++++++++
> > .../alloca/tst.alloca-struct-union-store.d | 39 ++++++++++++++++++
> > 9 files changed, 281 insertions(+), 9 deletions(-)
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-array-load.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-array-store.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-deref-load.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-deref-store.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-load.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-store.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-union-load.d
> > create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-union-store.d
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 5c7e3998..4e163e01 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -3051,7 +3051,7 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
> > int dreg = dst->dn_reg;
> > /*
> > - * If we're loading a bit-field, the size of our store is found by
> > + * If we're storing into a bit-field, the size of our store is found by
> > * rounding dst's cte_bits up to a byte boundary and then finding the
> > * nearest power of two to this value (see clp2(), above).
> > */
> > @@ -3062,13 +3062,15 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
> > size = dt_node_type_size(dst);
> > /*
> > - * If we're loading a writable non-alloca lvalue, and it's a
> > - * dereference, and *its* child is an alloca pointer, then this is a
> > - * dereferenced alloca pointer and needs bounds-checking (which could
> > - * not be done at deref time due to not knowing the size of the write).
> > + * If we're storing into a writable lvalue that is a dereference of an
> > + * alloca pointer or if we are storing into a writable alloca lvalue,
> > + * we need to do bounds-checking and turn the offset value into a real
> > + * pointer.
> > */
> > - if (dst->dn_flags & DT_NF_WRITABLE && dst->dn_flags & DT_NF_LVALUE
> > - && dst->dn_op == DT_TOK_DEREF && dst->dn_child->dn_flags & DT_NF_ALLOCA) {
> > + if (dst->dn_flags & DT_NF_WRITABLE && dst->dn_flags & DT_NF_LVALUE &&
> > + ((dst->dn_op == DT_TOK_DEREF &&
> > + dst->dn_child->dn_flags & DT_NF_ALLOCA) ||
> > + dst->dn_flags & DT_NF_ALLOCA)) {
> > assert(!(dst->dn_flags & DT_NF_BITFIELD));
> > if ((dreg = dt_regset_alloc(drp)) == -1)
> > @@ -6473,7 +6475,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > }
> > if (m.ctm_offset != 0)
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, dnp->dn_left->dn_reg, m.ctm_offset / NBBY));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, m.ctm_offset / NBBY));
> > if (!(dnp->dn_flags & DT_NF_REF)) {
> > uint_t op;
> > @@ -6481,8 +6483,20 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > op = dt_cg_ldsize(dnp, ctfp, m.ctm_type, &size);
> > + /*
> > + * If the left-hand side of PTR or DOT is an alloca
> > + * pointer, bounds-check the pointer reference.
> > + */
> > + if (dnp->dn_left->dn_flags & DT_NF_ALLOCA) {
> > + assert(dnp->dn_flags & DT_NF_ALLOCA);
> > + dt_cg_alloca_access_check(dlp, drp, dnp->dn_reg,
> > + DT_ISIMM, size);
> > + dt_cg_alloca_ptr(dlp, drp, dnp->dn_reg,
> > + dnp->dn_reg);
> > + }
> > +
> > if (dnp->dn_left->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
> > - emit(dlp, BPF_LOAD(op, dnp->dn_left->dn_reg, dnp->dn_left->dn_reg, 0));
> > + emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
> > else
> > dt_cg_load_scalar(dnp->dn_left, op, size, dlp, drp);
> > }
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-array-load.d b/test/unittest/funcs/alloca/tst.alloca-array-load.d
> > new file mode 100644
> > index 00000000..3db98049
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-array-load.d
> > @@ -0,0 +1,29 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Loading from array elements works for alloca pointers.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +int *arr;
> > +
> > +BEGIN
> > +{
> > + arr = alloca(5 * sizeof(int));
> > + arr[3] = 0x42;
> > + trace(arr[3]);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-array-store.d b/test/unittest/funcs/alloca/tst.alloca-array-store.d
> > new file mode 100644
> > index 00000000..17edb584
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-array-store.d
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Storing to array elements works for alloca pointers.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +int *arr;
> > +
> > +BEGIN
> > +{
> > + arr = alloca(5 * sizeof(int));
> > + arr[3] = 0x42;
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-deref-load.d b/test/unittest/funcs/alloca/tst.alloca-deref-load.d
> > new file mode 100644
> > index 00000000..2d23cdd3
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-deref-load.d
> > @@ -0,0 +1,29 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Loading from dereferenced alloca pointers works.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +int *val;
> > +
> > +BEGIN
> > +{
> > + val = alloca(sizeof(int));
> > + *val = 0x42;
> > + trace(*val);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-deref-store.d b/test/unittest/funcs/alloca/tst.alloca-deref-store.d
> > new file mode 100644
> > index 00000000..e7d36e53
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-deref-store.d
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Storing to dereferenced alloca pointers works.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +int *val;
> > +
> > +BEGIN
> > +{
> > + val = alloca(sizeof(int));
> > + *val = 0x42;
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-load.d b/test/unittest/funcs/alloca/tst.alloca-struct-load.d
> > new file mode 100644
> > index 00000000..5f022985
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-struct-load.d
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Loading from struct members works for alloca pointers.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +struct foo {
> > + int a;
> > + char b;
> > + int c;
> > +} *ptr;
> > +
> > +BEGIN
> > +{
> > + ptr = alloca(sizeof(struct foo));
> > + ptr->c = 0x42;
> > + trace(ptr->c);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-store.d b/test/unittest/funcs/alloca/tst.alloca-struct-store.d
> > new file mode 100644
> > index 00000000..4f55f958
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-struct-store.d
> > @@ -0,0 +1,32 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Storing to struct members works for alloca pointers.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +struct foo {
> > + int a;
> > + char b;
> > + int c;
> > +} *ptr;
> > +
> > +BEGIN
> > +{
> > + ptr = alloca(sizeof(struct foo));
> > + ptr->c = 0x42;
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-union-load.d b/test/unittest/funcs/alloca/tst.alloca-struct-union-load.d
> > new file mode 100644
> > index 00000000..7361b494
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-struct-union-load.d
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Loading from nested members works for alloca pointers.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +struct st {
> > + struct {
> > + union {
> > + struct {
> > + int a;
> > + char b;
> > + int c;
> > + } apa;
> > + uint64_t val;
> > + } u;
> > + } s;
> > +} *ptr;
> > +
> > +BEGIN
> > +{
> > + ptr = alloca(sizeof(struct st));
> > + ptr->s.u.apa.c = 0x42;
> > + trace(ptr->s.u.apa.c);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-union-store.d b/test/unittest/funcs/alloca/tst.alloca-struct-union-store.d
> > new file mode 100644
> > index 00000000..c0cbd787
> > --- /dev/null
> > +++ b/test/unittest/funcs/alloca/tst.alloca-struct-union-store.d
> > @@ -0,0 +1,39 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, 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.
> > + */
> > +
> > +/*
> > + * ASSERTION: Storing to nested members works for alloca pointers.
> > + *
> > + * SECTION: Actions and Subroutines/alloca()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +struct st {
> > + struct {
> > + union {
> > + struct {
> > + int a;
> > + char b;
> > + int c;
> > + } apa;
> > + uint64_t val;
> > + } u;
> > + } s;
> > +} *ptr;
> > +
> > +BEGIN
> > +{
> > + ptr = alloca(sizeof(struct st));
> > + ptr->s.u.apa.c = 0x42;
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
>
> _______________________________________________
> 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