Re: [PATCH 01/22] test: Handle dtrace:::ERROR arg3 specially

[Kris Van Hees <kris.van.hees@oracle.com>]

On Thu, Aug 29, 2024 at 01:21:58AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The ERROR probe's arg3 reports the culprit PC, whose value can vary
> with minor implementation changes.  On the one hand, we do not want
> tests to be overly sensitive to this value.  On the other hand, we
> do at least want to check the value is reasonable.
> 
> Therefore:
> 
> *)  Change tests that dump ERROR's args to omit arg3.
> 
> *)  Add a new test that checks that ERROR's arg3 is reasonable.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>
> ---
>  test/unittest/error/tst.DTRACEFLT_UNKNOWN.d   |  6 +-
>  test/unittest/error/tst.DTRACEFLT_UNKNOWN.r   |  2 +-
>  .../regression/tst.DTRACEFLT_BADADDR.d_path.d |  8 +-
>  .../regression/tst.DTRACEFLT_BADADDR.d_path.r |  2 +-
>  test/unittest/variables/bvar/tst.arg3-ERROR.d | 92 +++++++++++++++++++
>  5 files changed, 101 insertions(+), 9 deletions(-)
>  create mode 100644 test/unittest/variables/bvar/tst.arg3-ERROR.d
> 
> diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> index 001903ff..bfc77bf5 100644
> --- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> +++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2024, 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.
>   */
> @@ -19,8 +19,8 @@
>  
>  ERROR
>  {
> -	printf("The arguments are %u %u %u %u %u\n",
> -		arg1, arg2, arg3, arg4, arg5);
> +	printf("The arguments are %u %u PC %u %u\n",
> +		arg1, arg2, arg4, arg5);
>  	printf("The value of arg4 = %u\n", DTRACEFLT_UNKNOWN);
>  	exit(0);
>  }
> diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> index b11f6c99..3e7caac4 100644
> --- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> +++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> @@ -1,4 +1,4 @@
> -The arguments are 2 2 4 1 64
> +The arguments are 2 2 PC 1 64
>  The value of arg4 = 0
>  
>  -- @@stderr --
> diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> index c23f9503..ec519f4f 100644
> --- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> +++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> @@ -1,10 +1,10 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2015, 2024, 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.
>   */
> -/* @@xfail: dtv2 */
> +/* @@xfail: dtv2 d_path */
>  
>  /*
>   * ASSERTION:
> @@ -18,8 +18,7 @@
>  
>  ERROR
>  {
> -	printf("The arguments are %u %u %u %u %u\n",
> -		arg1, arg2, arg3, arg4, arg5);
> +	printf("The arguments are %u %u PC %u %u\n", arg1, arg2, arg4, arg5);
>  	printf("The value of arg4 should be %u\n", DTRACEFLT_BADADDR);
>  	printf("The value of arg5 should be %u\n", 0x18);
>  	exit(0);
> @@ -29,4 +28,5 @@ BEGIN
>  {
>  	d = d_path((struct path *)0x18);
>  	trace(d);
> +	exit(1);
>  }
> diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> index 8c601a43..be1f6d5b 100644
> --- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> +++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> @@ -1,4 +1,4 @@
> -The arguments are 2 1 28 1 24
> +The arguments are 2 1 PC 1 24
>  The value of arg4 should be 1
>  The value of arg5 should be 24
>  
> diff --git a/test/unittest/variables/bvar/tst.arg3-ERROR.d b/test/unittest/variables/bvar/tst.arg3-ERROR.d
> new file mode 100644
> index 00000000..6c2f5206
> --- /dev/null
> +++ b/test/unittest/variables/bvar/tst.arg3-ERROR.d
> @@ -0,0 +1,92 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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 'arg3' variable can be accessed in the ERROR probe
> + *	and its value (PC) is reasonable.
> + *
> + * SECTION: Variables/Built-in Variables/arg3
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	nerrs = 0;
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[0] */
> +	trace(y);
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[1] */
> +	trace(y);
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[2] */
> +	trace(y);
> +}
> +
> +BEGIN
> +{
> +	x = (int *)64;
> +	y = *x;                       /* Error at PC[3] */
> +	trace(y);
> +}
> +
> +ERROR
> +{
> +	/* Record the problematic PC and continue execution. */
> +	PC[nerrs++] = arg3;
> +}
> +
> +/*
> + * The problematic PCs are likely to satisfy the following
> + * reasonable checks, though it's possible that some radically
> + * different implementation in the future might violate one or
> + * more checks.
> + *
> + * The first problematic PC is expected in the some range.

"in the same range" ??  Changing to "some range".

> + *
> + * The next problematic PC is expected to be PC[0] plus some
> + * delta, including some special functions that are loaded.
> + *
> + * Then, the next PC is expected to be PC[1] plus some delta
> + * that is smaller and narrower since those special functions
> + * do not need to be reloaded.
> + *
> + * The last PC is expected to be PC[2] plus some predictable,
> + * small and narrow, delta PC[2]-PC[1].
> + */
> +BEGIN
> +/PC[0] >   100 &&
> + PC[0] <  2000 &&
> + PC[1] >  PC[0] +  100 &&
> + PC[1] <  PC[0] + 2000 &&
> + PC[2] >  PC[1] +   30 &&
> + PC[2] <  PC[1] +  500 &&
> + PC[3] == PC[2] + (PC[2] - PC[1])/

These ranges seem quite big.  I realize that code generator changes and changes
to linked in code can cause these error locations to drift over time, but I
think I'd rather have smaller ranges with the downfall of needing to update the
test at some point rather than having ranges that are so big that the test
itself may not really verify much anymore.

For that matter, perhaps it would be better to rework the test to actually
compare the error locations with the disassembler output because we know the
mechanism that sets the PC for the error location and thus we know what
instructions to expect there.  That would allow exact verification of the PC
values.

> +{
> +	printf("PCs: %d %d %d %d\n", PC[0], PC[1], PC[2], PC[3]);
> +	exit(0);
> +}
> +
> +BEGIN
> +{
> +	printf("ERROR!  PCs do not seem reasonable: %d %d %d %d\n",
> +		   PC[0], PC[1], PC[2], PC[3]);
> +	exit(1);
> +}
> -- 
> 2.43.5
>