public inbox for dtrace@lists.linux.dev
 help / color / mirror / Atom feed
* [PATCH 1/4] parser, cg: Implement the return() action
@ 2025-07-15  5:47 Kris Van Hees
  2025-07-15 10:32 ` [DTrace-devel] " Nick Alcock
  0 siblings, 1 reply; 3+ messages in thread
From: Kris Van Hees @ 2025-07-15  5:47 UTC (permalink / raw)
  To: dtrace, dtrace-devel

The return(n) action can be used for error injection by forcing a
given return value for a kernel function.

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 include/dtrace/actions_defines.h              |  1 +
 libdtrace/dt_cg.c                             | 21 +++++++++++++++
 libdtrace/dt_grammar.y                        |  1 -
 libdtrace/dt_impl.h                           |  3 ++-
 libdtrace/dt_lex.l                            |  1 -
 libdtrace/dt_open.c                           |  2 ++
 test/modules                                  |  1 +
 .../actions/return/err.D_PROTO_ARG.str.d      | 19 +++++++++++++
 .../actions/return/err.D_PROTO_ARG.str.r      |  4 +++
 .../actions/return/err.D_PROTO_ARG.void.d     | 19 +++++++++++++
 .../actions/return/err.D_PROTO_ARG.void.r     |  4 +++
 .../return/err.D_PROTO_LEN.missing_arg.d      | 20 ++++++++++++++
 .../return/err.D_PROTO_LEN.missing_arg.r      |  2 ++
 .../return/err.D_PROTO_LEN.too_many_args.d    | 20 ++++++++++++++
 .../return/err.D_PROTO_LEN.too_many_args.r    |  2 ++
 .../unittest/actions/return/err.destructive.d | 26 ++++++++++++++++++
 .../unittest/actions/return/err.destructive.r |  2 ++
 .../unittest/actions/return/tst.destructive.d | 27 +++++++++++++++++++
 .../unittest/actions/return/tst.destructive.r |  1 +
 19 files changed, 173 insertions(+), 3 deletions(-)
 create mode 100644 test/unittest/actions/return/err.D_PROTO_ARG.str.d
 create mode 100644 test/unittest/actions/return/err.D_PROTO_ARG.str.r
 create mode 100644 test/unittest/actions/return/err.D_PROTO_ARG.void.d
 create mode 100644 test/unittest/actions/return/err.D_PROTO_ARG.void.r
 create mode 100644 test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
 create mode 100644 test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.r
 create mode 100644 test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d
 create mode 100644 test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
 create mode 100644 test/unittest/actions/return/err.destructive.d
 create mode 100644 test/unittest/actions/return/err.destructive.r
 create mode 100644 test/unittest/actions/return/tst.destructive.d
 create mode 100644 test/unittest/actions/return/tst.destructive.r

diff --git a/include/dtrace/actions_defines.h b/include/dtrace/actions_defines.h
index 4ff4fb83..601328b8 100644
--- a/include/dtrace/actions_defines.h
+++ b/include/dtrace/actions_defines.h
@@ -51,6 +51,7 @@
 #define DTRACEACT_RAISE			(DTRACEACT_PROC_DESTRUCTIVE + 2)
 #define DTRACEACT_SYSTEM		(DTRACEACT_PROC_DESTRUCTIVE + 3)
 #define DTRACEACT_FREOPEN		(DTRACEACT_PROC_DESTRUCTIVE + 4)
+#define DTRACEACT_RETURN		(DTRACEACT_PROC_DESTRUCTIVE + 5)
 
 #define DTRACEACT_PROC_CONTROL		0x0300
 
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index bd0763d6..d80b0a55 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -2893,6 +2893,25 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
 	dt_regset_free(drp, BPF_REG_0);
 }
 
+static void
+dt_cg_act_return(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
+{
+	dt_irlist_t	*dlp = &pcb->pcb_ir;
+	dt_regset_t	*drp = pcb->pcb_regs;
+
+	dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
+
+	if (dt_regset_xalloc_args(drp) == -1)
+		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
+	emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
+	emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_CTX));
+	emit(dlp, BPF_MOV_REG(BPF_REG_2, dnp->dn_args->dn_reg));
+	dt_regset_xalloc(drp, BPF_REG_0);
+	emit(dlp, BPF_CALL_HELPER(BPF_FUNC_override_return));
+	dt_regset_free_args(drp);
+	dt_regset_free(drp, BPF_REG_0);
+}
+
 typedef void dt_cg_action_f(dt_pcb_t *, dt_node_t *, dtrace_actkind_t);
 
 typedef struct dt_cg_actdesc {
@@ -2951,6 +2970,8 @@ static const dt_cg_actdesc_t _dt_cg_actions[DT_ACT_MAX] = {
 	[DT_ACT_IDX(DT_ACT_SETOPT)]		= { &dt_cg_act_setopt, },
 	[DT_ACT_IDX(DT_ACT_PCAP)]		= { &dt_cg_act_pcap, },
 	[DT_ACT_IDX(DT_ACT_PRINT)]		= { &dt_cg_act_print, },
+	[DT_ACT_IDX(DT_ACT_RETURN)]		= { &dt_cg_act_return,
+						    DTRACEACT_RETURN },
 };
 
 dt_irnode_t *
diff --git a/libdtrace/dt_grammar.y b/libdtrace/dt_grammar.y
index 048a974d..a0911097 100644
--- a/libdtrace/dt_grammar.y
+++ b/libdtrace/dt_grammar.y
@@ -73,7 +73,6 @@ int yylex (void);
 %token	DT_KEY_PROVIDER
 %token	DT_KEY_REGISTER
 %token	DT_KEY_RESTRICT
-%token	DT_KEY_RETURN
 %token	DT_KEY_SELF
 %token	DT_KEY_SHORT
 %token	DT_KEY_SIGNED
diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 2adc1252..e7192290 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -550,8 +550,9 @@ struct dtrace_hdl {
 #define	DT_ACT_SETOPT		DT_ACT(28)	/* setopt() action */
 #define	DT_ACT_PCAP		DT_ACT(29)	/* pcap() action */
 #define	DT_ACT_PRINT		DT_ACT(30)	/* print() action */
+#define	DT_ACT_RETURN		DT_ACT(31)	/* return() action */
 
-#define DT_ACT_MAX		31
+#define DT_ACT_MAX		32
 
 /*
  * Sentinel to tell freopen() to restore the saved stdout.  This must not
diff --git a/libdtrace/dt_lex.l b/libdtrace/dt_lex.l
index cc165c1e..bdcca415 100644
--- a/libdtrace/dt_lex.l
+++ b/libdtrace/dt_lex.l
@@ -104,7 +104,6 @@ if (yypcb->pcb_token != 0) {
 <S0>provider	return DT_KEY_PROVIDER;
 <S0>register	return DT_KEY_REGISTER;
 <S0>restrict	return DT_KEY_RESTRICT;
-<S0>return	return DT_KEY_RETURN;
 <S0>self	return DT_KEY_SELF;
 <S0>short	return DT_KEY_SHORT;
 <S0>signed	return DT_KEY_SIGNED;
diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
index c67455e7..08394a11 100644
--- a/libdtrace/dt_open.c
+++ b/libdtrace/dt_open.c
@@ -269,6 +269,8 @@ static const dt_ident_t _dtrace_globals[] = {
 	&dt_idops_func, "void(int)" },
 { "rand", DT_IDENT_FUNC, 0, DIF_SUBR_RAND, DT_ATTR_STABCMN, DT_VERS_1_0,
 	&dt_idops_func, "int()" },
+{ "return", DT_IDENT_ACTFUNC, 0, DT_ACT_RETURN, DT_ATTR_STABCMN, DT_VERS_2_0,
+	&dt_idops_func, "void(int)" },
 { "rindex", DT_IDENT_FUNC, 0, DIF_SUBR_RINDEX, DT_ATTR_STABCMN, DT_VERS_1_1,
 	&dt_idops_func, "int(const char *, const char *, [int])" },
 { "rw_iswriter", DT_IDENT_FUNC, 0, DIF_SUBR_RW_ISWRITER,
diff --git a/test/modules b/test/modules
index 0f01d6e0..7782540c 100644
--- a/test/modules
+++ b/test/modules
@@ -1,3 +1,4 @@
+btrfs
 ext4
 isofs
 nfs
diff --git a/test/unittest/actions/return/err.D_PROTO_ARG.str.d b/test/unittest/actions/return/err.D_PROTO_ARG.str.d
new file mode 100644
index 00000000..d04836f3
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_ARG.str.d
@@ -0,0 +1,19 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: The return() action cannot be passed a string argument.
+ *
+ * SECTION: Actions and Subroutines/return()
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	return("");
+}
diff --git a/test/unittest/actions/return/err.D_PROTO_ARG.str.r b/test/unittest/actions/return/err.D_PROTO_ARG.str.r
new file mode 100644
index 00000000..46ca1828
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_ARG.str.r
@@ -0,0 +1,4 @@
+-- @@stderr --
+dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_ARG.str.d: [D_PROTO_ARG] line 18: return( ) argument #1 is incompatible with prototype:
+	prototype: int
+	 argument: string
diff --git a/test/unittest/actions/return/err.D_PROTO_ARG.void.d b/test/unittest/actions/return/err.D_PROTO_ARG.void.d
new file mode 100644
index 00000000..dc3b15fa
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_ARG.void.d
@@ -0,0 +1,19 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: The return() action cannot be passed a void argument.
+ *
+ * SECTION: Actions and Subroutines/return()
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	return(return(0));
+}
diff --git a/test/unittest/actions/return/err.D_PROTO_ARG.void.r b/test/unittest/actions/return/err.D_PROTO_ARG.void.r
new file mode 100644
index 00000000..1573c557
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_ARG.void.r
@@ -0,0 +1,4 @@
+-- @@stderr --
+dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_ARG.void.d: [D_PROTO_ARG] line 18: return( ) argument #1 is incompatible with prototype:
+	prototype: int
+	 argument: void
diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
new file mode 100644
index 00000000..97ed0762
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
@@ -0,0 +1,20 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: The return() action takes exactly one argument.
+ *
+ * SECTION: Actions and Subroutines/return()
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	return();
+	exit(0);
+}
diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.r b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.r
new file mode 100644
index 00000000..fd7c2512
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.r
@@ -0,0 +1,2 @@
+-- @@stderr --
+dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d: [D_PROTO_LEN] line 18: return( ) prototype mismatch: 0 args passed, 1 expected
diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d
new file mode 100644
index 00000000..0776192b
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d
@@ -0,0 +1,20 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: The return() action takes exactly one argument.
+ *
+ * SECTION: Actions and Subroutines/return()
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	return(1, 2);
+	exit(0);
+}
diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
new file mode 100644
index 00000000..37ff4d9d
--- /dev/null
+++ b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
@@ -0,0 +1,2 @@
+-- @@stderr --
+dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d: [D_PROTO_LEN] line 18: return( ) prototype mismatch: 2 args passed, 1 expected
diff --git a/test/unittest/actions/return/err.destructive.d b/test/unittest/actions/return/err.destructive.d
new file mode 100644
index 00000000..4c4a246a
--- /dev/null
+++ b/test/unittest/actions/return/err.destructive.d
@@ -0,0 +1,26 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: return() is allowed when destructive execution is allowed
+ *
+ * SECTION: Actions and Subroutines/return()
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	ok = 0;
+	exit(0);
+}
+
+rawfbt:btrfs:open_ctree:entry
+/ok/
+{
+	return(0);
+}
diff --git a/test/unittest/actions/return/err.destructive.r b/test/unittest/actions/return/err.destructive.r
new file mode 100644
index 00000000..004c30a9
--- /dev/null
+++ b/test/unittest/actions/return/err.destructive.r
@@ -0,0 +1,2 @@
+-- @@stderr --
+dtrace: could not enable tracing: Destructive actions not allowed
diff --git a/test/unittest/actions/return/tst.destructive.d b/test/unittest/actions/return/tst.destructive.d
new file mode 100644
index 00000000..f1d9e4d3
--- /dev/null
+++ b/test/unittest/actions/return/tst.destructive.d
@@ -0,0 +1,27 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, 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: return() is allowed when destructive execution is allowed
+ *
+ * SECTION: Actions and Subroutines/return()
+ */
+
+#pragma D option quiet
+#pragma D option destructive
+
+BEGIN
+{
+	ok = 0;
+	exit(0);
+}
+
+rawfbt:btrfs:open_ctree:entry
+/ok/
+{
+	return(0);
+}
diff --git a/test/unittest/actions/return/tst.destructive.r b/test/unittest/actions/return/tst.destructive.r
new file mode 100644
index 00000000..8b137891
--- /dev/null
+++ b/test/unittest/actions/return/tst.destructive.r
@@ -0,0 +1 @@
+
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [DTrace-devel] [PATCH 1/4] parser, cg: Implement the return() action
  2025-07-15  5:47 [PATCH 1/4] parser, cg: Implement the return() action Kris Van Hees
@ 2025-07-15 10:32 ` Nick Alcock
  2025-07-15 15:03   ` Kris Van Hees
  0 siblings, 1 reply; 3+ messages in thread
From: Nick Alcock @ 2025-07-15 10:32 UTC (permalink / raw)
  To: Kris Van Hees, dtrace, dtrace-devel

On 15 Jul 2025, Kris Van Hees via DTrace-devel outgrape:

> The return(n) action can be used for error injection by forcing a
> given return value for a kernel function.

I am amazed this is even possible! I can see all sorts of uses.

Presumably we need to tell people to turn on CONFIG_BPF_KPROBE_OVERRIDE
somewhere... UEK already has it on, good. Not many functions
covered... I suppose it'll let them trace a *few* functions, and if they
use this they're probably used to hacking in more error-injection points
anyway.

> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

modulo the tiny nits below.

> diff --git a/libdtrace/dt_lex.l b/libdtrace/dt_lex.l
> index cc165c1e..bdcca415 100644
> --- a/libdtrace/dt_lex.l
> +++ b/libdtrace/dt_lex.l
> @@ -104,7 +104,6 @@ if (yypcb->pcb_token != 0) {
>  <S0>provider	return DT_KEY_PROVIDER;
>  <S0>register	return DT_KEY_REGISTER;
>  <S0>restrict	return DT_KEY_RESTRICT;
> -<S0>return	return DT_KEY_RETURN;
>  <S0>self	return DT_KEY_SELF;
>  <S0>short	return DT_KEY_SHORT;
>  <S0>signed	return DT_KEY_SIGNED;

I guess these are here for C/C++ header parsing :) a somewhat hopeful
approach. I wonder if this change will cause breakage for non-bracketed
returns in C code in static inlines... and returns of non-integral
types, and just plain return... ugh. I suppose inlines barely work
anyway and the right way to do that is to simply have the parser ignore
static inlines in C headers anyway, not have it try to parse them :)

> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index c67455e7..08394a11 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -269,6 +269,8 @@ static const dt_ident_t _dtrace_globals[] = {
>  	&dt_idops_func, "void(int)" },
>  { "rand", DT_IDENT_FUNC, 0, DIF_SUBR_RAND, DT_ATTR_STABCMN, DT_VERS_1_0,
>  	&dt_idops_func, "int()" },
> +{ "return", DT_IDENT_ACTFUNC, 0, DT_ACT_RETURN, DT_ATTR_STABCMN, DT_VERS_2_0,
> +	&dt_idops_func, "void(int)" },

The "problem" (if this actually is a problem) is that this is not the C
prototype, which is not expressible in C :(

> diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
> new file mode 100644
> index 00000000..97ed0762
> --- /dev/null
> +++ b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
> @@ -0,0 +1,20 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2025, 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: The return() action takes exactly one argument.
> + *
> + * SECTION: Actions and Subroutines/return()
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	return();
> +	exit(0);
> +}

You might want to try a straight 'return;' as well :)

> diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
> new file mode 100644
> index 00000000..37ff4d9d
> --- /dev/null
> +++ b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
> @@ -0,0 +1,2 @@
> +-- @@stderr --
> +dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d: [D_PROTO_LEN] line 18: return( ) prototype mismatch: 2 args passed, 1 expected
> diff --git a/test/unittest/actions/return/err.destructive.d b/test/unittest/actions/return/err.destructive.d
> new file mode 100644
> index 00000000..4c4a246a
> --- /dev/null
> +++ b/test/unittest/actions/return/err.destructive.d
> @@ -0,0 +1,26 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2025, 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: return() is allowed when destructive execution is allowed

return() is forbidden when destructive execution is not allowed.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [DTrace-devel] [PATCH 1/4] parser, cg: Implement the return() action
  2025-07-15 10:32 ` [DTrace-devel] " Nick Alcock
@ 2025-07-15 15:03   ` Kris Van Hees
  0 siblings, 0 replies; 3+ messages in thread
From: Kris Van Hees @ 2025-07-15 15:03 UTC (permalink / raw)
  To: Nick Alcock; +Cc: Kris Van Hees, dtrace, dtrace-devel

On Tue, Jul 15, 2025 at 11:32:42AM +0100, Nick Alcock wrote:
> On 15 Jul 2025, Kris Van Hees via DTrace-devel outgrape:
> 
> > The return(n) action can be used for error injection by forcing a
> > given return value for a kernel function.
> 
> I am amazed this is even possible! I can see all sorts of uses.
> 
> Presumably we need to tell people to turn on CONFIG_BPF_KPROBE_OVERRIDE
> somewhere... UEK already has it on, good. Not many functions
> covered... I suppose it'll let them trace a *few* functions, and if they
> use this they're probably used to hacking in more error-injection points
> anyway.

Well, it will be mentioned in the release notes of course, but the way it is
implemented, if the kernel is not configured to support it, there will not be
any functions for which it will be allowed (see the rest of the series).

I debated putting it in its own provider but that seemms less useful, even
though it would mean you can list which functions can support this.  That may
need to be something for the future if it is wanted - right now it is easy
enuogh for someone to look in /sys/kernel/debug/error_injection/list for that
info.  People using this feature are expected to really know what they are
doing :)

> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> 
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
> 
> modulo the tiny nits below.
> 
> > diff --git a/libdtrace/dt_lex.l b/libdtrace/dt_lex.l
> > index cc165c1e..bdcca415 100644
> > --- a/libdtrace/dt_lex.l
> > +++ b/libdtrace/dt_lex.l
> > @@ -104,7 +104,6 @@ if (yypcb->pcb_token != 0) {
> >  <S0>provider	return DT_KEY_PROVIDER;
> >  <S0>register	return DT_KEY_REGISTER;
> >  <S0>restrict	return DT_KEY_RESTRICT;
> > -<S0>return	return DT_KEY_RETURN;
> >  <S0>self	return DT_KEY_SELF;
> >  <S0>short	return DT_KEY_SHORT;
> >  <S0>signed	return DT_KEY_SIGNED;
> 
> I guess these are here for C/C++ header parsing :) a somewhat hopeful
> approach. I wonder if this change will cause breakage for non-bracketed
> returns in C code in static inlines... and returns of non-integral
> types, and just plain return... ugh. I suppose inlines barely work
> anyway and the right way to do that is to simply have the parser ignore
> static inlines in C headers anyway, not have it try to parse them :)

No, they are there as symbols reserved for future use.  So by removing return
here, we are putting that reserved symbol to use :)

> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index c67455e7..08394a11 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -269,6 +269,8 @@ static const dt_ident_t _dtrace_globals[] = {
> >  	&dt_idops_func, "void(int)" },
> >  { "rand", DT_IDENT_FUNC, 0, DIF_SUBR_RAND, DT_ATTR_STABCMN, DT_VERS_1_0,
> >  	&dt_idops_func, "int()" },
> > +{ "return", DT_IDENT_ACTFUNC, 0, DT_ACT_RETURN, DT_ATTR_STABCMN, DT_VERS_2_0,
> > +	&dt_idops_func, "void(int)" },
> 
> The "problem" (if this actually is a problem) is that this is not the C
> prototype, which is not expressible in C :(

Since return is being implemented as an action it needs to be void(int).  If
we were to implement it like its C counterpart, then it would be a language
construct, and in taht case I would expect it to somehow set a return value
for the clause. which doesn't work (clauses do not have return values), and
it would not accomplish what we need here.  Since you can never return from
a clause (perhaps if/when conditional expressions are implemented, we may
need to revisit this).

> > diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
> > new file mode 100644
> > index 00000000..97ed0762
> > --- /dev/null
> > +++ b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
> > @@ -0,0 +1,20 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2025, 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: The return() action takes exactly one argument.
> > + *
> > + * SECTION: Actions and Subroutines/return()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +BEGIN
> > +{
> > +	return();
> > +	exit(0);
> > +}
> 
> You might want to try a straight 'return;' as well :)

Sure.

> > diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
> > new file mode 100644
> > index 00000000..37ff4d9d
> > --- /dev/null
> > +++ b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
> > @@ -0,0 +1,2 @@
> > +-- @@stderr --
> > +dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d: [D_PROTO_LEN] line 18: return( ) prototype mismatch: 2 args passed, 1 expected
> > diff --git a/test/unittest/actions/return/err.destructive.d b/test/unittest/actions/return/err.destructive.d
> > new file mode 100644
> > index 00000000..4c4a246a
> > --- /dev/null
> > +++ b/test/unittest/actions/return/err.destructive.d
> > @@ -0,0 +1,26 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2025, 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: return() is allowed when destructive execution is allowed
> 
> return() is forbidden when destructive execution is not allowed.

Thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-07-15 15:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-15  5:47 [PATCH 1/4] parser, cg: Implement the return() action Kris Van Hees
2025-07-15 10:32 ` [DTrace-devel] " Nick Alcock
2025-07-15 15:03   ` Kris Van Hees

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox