[PATCH v2 0/2] riscv: simplify longjmp

[PATCH v2 0/2] riscv: simplify longjmp

[Heinrich Schuchardt]

The implementation of longjmp() is simplified.
A unit test for longjmp() is provided.

For testing use

    CONFIG_UNIT_TEST=y
    CONFIG_CMD_SETEXPR=n

and execute

    ut lib

v2:
	correct title of patch 1

Heinrich Schuchardt (2):
  riscv: simplify longjmp
  test: unit test for longjmp

 arch/riscv/lib/setjmp.S |  8 ++------
 test/lib/Makefile       |  1 +
 test/lib/longjmp.c      | 44 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)
 create mode 100644 test/lib/longjmp.c

--
2.30.2

[PATCH v2 1/2] riscv: simplify longjmp

[Heinrich Schuchardt]

The value returned by setjmp must be nonzero. If zero is passed as
parameter it must be replaced by 1.

This patch reduces the code size a bit.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Sean Anderson <seanga2@gmail.com>
---
v2:
	fix typo in title
---
 arch/riscv/lib/setjmp.S | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/lib/setjmp.S b/arch/riscv/lib/setjmp.S
index 72bc9241f6..99d6195827 100644
--- a/arch/riscv/lib/setjmp.S
+++ b/arch/riscv/lib/setjmp.S
@@ -54,12 +54,8 @@ ENTRY(longjmp)
 	LOAD_IDX(sp, 13)

 	/* Move the return value in place, but return 1 if passed 0. */
-	beq a1, zero, longjmp_1
-	mv a0, a1
-	ret
-
-	longjmp_1:
-	li a0, 1
+	seqz a0, a1
+	add a0, a0, a1
 	ret
 ENDPROC(longjmp)
 .popsection
--
2.30.2

[PATCH v2 1/2] riscv: simplify longjmp

[Sean Anderson]

On 3/22/21 7:02 AM, Heinrich Schuchardt wrote:
> The value returned by setjmp must be nonzero. If zero is passed as
> parameter it must be replaced by 1.
> 
> This patch reduces the code size a bit.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Sean Anderson <seanga2@gmail.com>

You are missing something here.

> ---
> v2:
> 	fix typo in title
> ---
>   arch/riscv/lib/setjmp.S | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/lib/setjmp.S b/arch/riscv/lib/setjmp.S
> index 72bc9241f6..99d6195827 100644
> --- a/arch/riscv/lib/setjmp.S
> +++ b/arch/riscv/lib/setjmp.S
> @@ -54,12 +54,8 @@ ENTRY(longjmp)
>   	LOAD_IDX(sp, 13)
> 
>   	/* Move the return value in place, but return 1 if passed 0. */
> -	beq a1, zero, longjmp_1
> -	mv a0, a1
> -	ret
> -
> -	longjmp_1:
> -	li a0, 1
> +	seqz a0, a1
> +	add a0, a0, a1
>   	ret
>   ENDPROC(longjmp)
>   .popsection
> --
> 2.30.2
> 

[PATCH v2 1/2] riscv: simplify longjmp

[Leo Liang]

Hi Heinrich,

On Mon, Mar 22, 2021 at 12:02:48PM +0100, Heinrich Schuchardt wrote:
> The value returned by setjmp must be nonzero. If zero is passed as
> parameter it must be replaced by 1.
> 
> This patch reduces the code size a bit.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Sean Anderson <seanga2@gmail.com>

I think Sean is refering to the "Reviewed-by" tag is missing.
Otherwise than that, LGTM.

> ---
> v2:
> 	fix typo in title
> ---
>  arch/riscv/lib/setjmp.S | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/lib/setjmp.S b/arch/riscv/lib/setjmp.S
> index 72bc9241f6..99d6195827 100644
> --- a/arch/riscv/lib/setjmp.S
> +++ b/arch/riscv/lib/setjmp.S
> @@ -54,12 +54,8 @@ ENTRY(longjmp)
>  	LOAD_IDX(sp, 13)
> 
>  	/* Move the return value in place, but return 1 if passed 0. */
> -	beq a1, zero, longjmp_1
> -	mv a0, a1
> -	ret
> -
> -	longjmp_1:
> -	li a0, 1
> +	seqz a0, a1
> +	add a0, a0, a1
>  	ret
>  ENDPROC(longjmp)
>  .popsection
> --
> 2.30.2
>
Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

Best regards,
Leo

[PATCH v2 2/2] test: unit test for longjmp

[Heinrich Schuchardt]

Provide a unit test for the longjmp() library function

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	no change
---
 test/lib/Makefile  |  1 +
 test/lib/longjmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 test/lib/longjmp.c

diff --git a/test/lib/Makefile b/test/lib/Makefile
index 97c11e35a8..a30f615aa9 100644
--- a/test/lib/Makefile
+++ b/test/lib/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
 obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
 obj-y += hexdump.o
 obj-y += lmb.o
+obj-y += longjmp.o
 obj-$(CONFIG_CONSOLE_RECORD) += test_print.o
 obj-$(CONFIG_SSCANF) += sscanf.o
 obj-y += string.o
diff --git a/test/lib/longjmp.c b/test/lib/longjmp.c
new file mode 100644
index 0000000000..7571540ffc
--- /dev/null
+++ b/test/lib/longjmp.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test setjmp(), longjmp()
+ *
+ * Copyright (c) 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ */
+
+#include <common.h>
+#include <test/lib.h>
+#include <test/test.h>
+#include <test/ut.h>
+#include <asm/setjmp.h>
+
+/**
+ * test_longjmp_ret() - get longjmp() return value
+ *
+ * @i:		value passed to longjmp()
+ * Return:	value returned by longjmp()
+ */
+int test_longjmp_ret(int i)
+{
+	jmp_buf env;
+	int ret;
+
+	ret = setjmp(env);
+	if (ret)
+		return ret;
+	longjmp(env, i);
+	/* We should not arrive here */
+	return 0x1000;
+}
+
+static int lib_test_longjmp(struct unit_test_state *uts)
+{
+	int i;
+
+	for (i = -3; i < 0; ++i)
+		ut_asserteq(i, test_longjmp_ret(i));
+	ut_asserteq(1, test_longjmp_ret(0));
+	for (i = 1; i < 4; ++i)
+		ut_asserteq(i, test_longjmp_ret(i));
+	return 0;
+}
+LIB_TEST(lib_test_longjmp, 0);
--
2.30.2

[PATCH v2 2/2] test: unit test for longjmp

[Sean Anderson]

On 3/22/21 7:02 AM, Heinrich Schuchardt wrote:
> Provide a unit test for the longjmp() library function
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
> 	no change
> ---
>   test/lib/Makefile  |  1 +
>   test/lib/longjmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
>   create mode 100644 test/lib/longjmp.c
> 
> diff --git a/test/lib/Makefile b/test/lib/Makefile
> index 97c11e35a8..a30f615aa9 100644
> --- a/test/lib/Makefile
> +++ b/test/lib/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
>   obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
>   obj-y += hexdump.o
>   obj-y += lmb.o
> +obj-y += longjmp.o
>   obj-$(CONFIG_CONSOLE_RECORD) += test_print.o
>   obj-$(CONFIG_SSCANF) += sscanf.o
>   obj-y += string.o
> diff --git a/test/lib/longjmp.c b/test/lib/longjmp.c
> new file mode 100644
> index 0000000000..7571540ffc
> --- /dev/null
> +++ b/test/lib/longjmp.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Test setjmp(), longjmp()
> + *
> + * Copyright (c) 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + */
> +
> +#include <common.h>
> +#include <test/lib.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +#include <asm/setjmp.h>
> +
> +/**
> + * test_longjmp_ret() - get longjmp() return value
> + *
> + * @i:		value passed to longjmp()
> + * Return:	value returned by longjmp()
> + */
> +int test_longjmp_ret(int i)
> +{
> +	jmp_buf env;
> +	int ret;
> +
> +	ret = setjmp(env);
> +	if (ret)
> +		return ret;
> +	longjmp(env, i);
> +	/* We should not arrive here */
> +	return 0x1000;
> +}
> +
> +static int lib_test_longjmp(struct unit_test_state *uts)
> +{
> +	int i;
> +
> +	for (i = -3; i < 0; ++i)
> +		ut_asserteq(i, test_longjmp_ret(i));
> +	ut_asserteq(1, test_longjmp_ret(0));
> +	for (i = 1; i < 4; ++i)
> +		ut_asserteq(i, test_longjmp_ret(i));
> +	return 0;
> +}
> +LIB_TEST(lib_test_longjmp, 0);
> --
> 2.30.2
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>
Tested-by: Sean Anderson <seanga2@gmail.com>

Though I would like to test that variables are set correctly e.g. by doing

int test_longjmp_ret(int i)
{
	jmp_buf env;
	int ret;

	ret = setjmp(env);
	if (ret)
		return ret;
	ret = 0x1000;
	longjmp(env, i);
	/* We should not arrive here */
	return ret;
}

--Sean

[PATCH v2 2/2] test: unit test for longjmp

[Sean Anderson]

On 3/22/21 9:23 AM, Sean Anderson wrote:
> 
> On 3/22/21 7:02 AM, Heinrich Schuchardt wrote:
>> Provide a unit test for the longjmp() library function
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2:
>>     no change
>> ---
>>   test/lib/Makefile  |  1 +
>>   test/lib/longjmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 45 insertions(+)
>>   create mode 100644 test/lib/longjmp.c
>>
>> diff --git a/test/lib/Makefile b/test/lib/Makefile
>> index 97c11e35a8..a30f615aa9 100644
>> --- a/test/lib/Makefile
>> +++ b/test/lib/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
>>   obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
>>   obj-y += hexdump.o
>>   obj-y += lmb.o
>> +obj-y += longjmp.o
>>   obj-$(CONFIG_CONSOLE_RECORD) += test_print.o
>>   obj-$(CONFIG_SSCANF) += sscanf.o
>>   obj-y += string.o
>> diff --git a/test/lib/longjmp.c b/test/lib/longjmp.c
>> new file mode 100644
>> index 0000000000..7571540ffc
>> --- /dev/null
>> +++ b/test/lib/longjmp.c
>> @@ -0,0 +1,44 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Test setjmp(), longjmp()
>> + *
>> + * Copyright (c) 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
>> + */
>> +
>> +#include <common.h>
>> +#include <test/lib.h>
>> +#include <test/test.h>
>> +#include <test/ut.h>
>> +#include <asm/setjmp.h>
>> +
>> +/**
>> + * test_longjmp_ret() - get longjmp() return value
>> + *
>> + * @i:        value passed to longjmp()
>> + * Return:    value returned by longjmp()
>> + */
>> +int test_longjmp_ret(int i)
>> +{
>> +    jmp_buf env;
>> +    int ret;
>> +
>> +    ret = setjmp(env);
>> +    if (ret)
>> +        return ret;
>> +    longjmp(env, i);
>> +    /* We should not arrive here */
>> +    return 0x1000;
>> +}
>> +
>> +static int lib_test_longjmp(struct unit_test_state *uts)
>> +{
>> +    int i;
>> +
>> +    for (i = -3; i < 0; ++i)
>> +        ut_asserteq(i, test_longjmp_ret(i));
>> +    ut_asserteq(1, test_longjmp_ret(0));
>> +    for (i = 1; i < 4; ++i)
>> +        ut_asserteq(i, test_longjmp_ret(i));
>> +    return 0;
>> +}
>> +LIB_TEST(lib_test_longjmp, 0);
>> -- 
>> 2.30.2
>>
> 
> Reviewed-by: Sean Anderson <seanga2@gmail.com>
> Tested-by: Sean Anderson <seanga2@gmail.com>
> 
> Though I would like to test that variables are set correctly e.g. by doing
> 
> int test_longjmp_ret(int i)
> {
>      jmp_buf env;
>      int ret;
> 
>      ret = setjmp(env);
>      if (ret)
>          return ret;
>      ret = 0x1000;
>      longjmp(env, i);
>      /* We should not arrive here */
>      return ret;
> }
> 
> --Sean

err, rather by doing

int test_longjmp_ret(int i)
{
      jmp_buf env;
      int ret;
      int foo = i;

      ret = setjmp(env);
      if (ret)
          return foo;
      foo = 0x1000;
      longjmp(env, i);
      /* We should not arrive here */
      return foo;
}

or something else which demonstrates that variables get reset to their
earlier values.

--Sean

[PATCH v2 2/2] test: unit test for longjmp

[Heinrich Schuchardt]

On 22.03.21 14:30, Sean Anderson wrote:
>
> On 3/22/21 9:23 AM, Sean Anderson wrote:
>>
>> On 3/22/21 7:02 AM, Heinrich Schuchardt wrote:
>>> Provide a unit test for the longjmp() library function
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>> v2:
>>> ??? no change
>>> ---
>>> ? test/lib/Makefile? |? 1 +
>>> ? test/lib/longjmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>> ? 2 files changed, 45 insertions(+)
>>> ? create mode 100644 test/lib/longjmp.c
>>>
>>> diff --git a/test/lib/Makefile b/test/lib/Makefile
>>> index 97c11e35a8..a30f615aa9 100644
>>> --- a/test/lib/Makefile
>>> +++ b/test/lib/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
>>> ? obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
>>> ? obj-y += hexdump.o
>>> ? obj-y += lmb.o
>>> +obj-y += longjmp.o
>>> ? obj-$(CONFIG_CONSOLE_RECORD) += test_print.o
>>> ? obj-$(CONFIG_SSCANF) += sscanf.o
>>> ? obj-y += string.o
>>> diff --git a/test/lib/longjmp.c b/test/lib/longjmp.c
>>> new file mode 100644
>>> index 0000000000..7571540ffc
>>> --- /dev/null
>>> +++ b/test/lib/longjmp.c
>>> @@ -0,0 +1,44 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Test setjmp(), longjmp()
>>> + *
>>> + * Copyright (c) 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <test/lib.h>
>>> +#include <test/test.h>
>>> +#include <test/ut.h>
>>> +#include <asm/setjmp.h>
>>> +
>>> +/**
>>> + * test_longjmp_ret() - get longjmp() return value
>>> + *
>>> + * @i:??????? value passed to longjmp()
>>> + * Return:??? value returned by longjmp()
>>> + */
>>> +int test_longjmp_ret(int i)
>>> +{
>>> +??? jmp_buf env;
>>> +??? int ret;
>>> +
>>> +??? ret = setjmp(env);
>>> +??? if (ret)
>>> +??????? return ret;
>>> +??? longjmp(env, i);
>>> +??? /* We should not arrive here */
>>> +??? return 0x1000;
>>> +}
>>> +
>>> +static int lib_test_longjmp(struct unit_test_state *uts)
>>> +{
>>> +??? int i;
>>> +
>>> +??? for (i = -3; i < 0; ++i)
>>> +??????? ut_asserteq(i, test_longjmp_ret(i));
>>> +??? ut_asserteq(1, test_longjmp_ret(0));
>>> +??? for (i = 1; i < 4; ++i)
>>> +??????? ut_asserteq(i, test_longjmp_ret(i));
>>> +??? return 0;
>>> +}
>>> +LIB_TEST(lib_test_longjmp, 0);
>>> --?
>>> 2.30.2
>>>
>>
>> Reviewed-by: Sean Anderson <seanga2@gmail.com>
>> Tested-by: Sean Anderson <seanga2@gmail.com>
>>
>> Though I would like to test that variables are set correctly e.g. by
>> doing
>>
>> int test_longjmp_ret(int i)
>> {
>> ???? jmp_buf env;
>> ???? int ret;
>>
>> ???? ret = setjmp(env);
>> ???? if (ret)
>> ???????? return ret;
>> ???? ret = 0x1000;
>> ???? longjmp(env, i);
>> ???? /* We should not arrive here */
>> ???? return ret;
>> }
>>
>> --Sean
>
> err, rather by doing
>
> int test_longjmp_ret(int i)
> {
> ???? jmp_buf env;
> ???? int ret;
> ???? int foo = i;
>
> ???? ret = setjmp(env);
> ???? if (ret)
> ???????? return foo;
> ???? foo = 0x1000;
> ???? longjmp(env, i);
> ???? /* We should not arrive here */
> ???? return foo;
> }
>
> or something else which demonstrates that variables get reset to their
> earlier values.
>
> --Sean

Hello Sean,

thank you for reviewing.

Would the following make sense to you to check that the stack pointer is
correctly restored?


struct test_jmp_buf {
        jmp_buf env;
        int val;
};

/**
 * test_longjmp() - test longjmp function
 *
 * @i is passed to longjmp.
 * @i << 8 is set in the environment structure.
 *
 * @env:        environment
 * @i:          value passed to longjmp()
 */
static void noinline test_longjmp(struct test_jmp_buf *env, int i)
{
        env->val = i << 8;
        longjmp(env->env, i);
}

/**
 * test_setjmp() - test setjmp function
 *
 * setjmp() will return the value @i passed to longjmp() if @i is non-zero.
 * For @i == 0 we expect return value 1.
 *
 * @i << 8 will be set by test_longjmp in the environment structure.
 * This value can be used to check that the stack frame is restored.
 *
 * We return the XORed values to allow simply check both@once.
 *
 * @i:          value passed to longjmp()
 * Return:      values return byby longjmp()
 */
static int test_setjmp(int i)
{
        struct test_jmp_buf env;
        int ret;

        env.val = -1;
        ret = setjmp(env.env);
        if (ret)
                return ret ^ env.val;
        test_longjmp(&env, i);
        /* We should not arrive here */
        return 0x1000;
}

Best regards

Heinrich

[PATCH v2 2/2] test: unit test for longjmp

[Sean Anderson]

On 3/22/21 12:42 PM, Heinrich Schuchardt wrote:
> On 22.03.21 14:30, Sean Anderson wrote:
>>
>> On 3/22/21 9:23 AM, Sean Anderson wrote:
>>>
>>> On 3/22/21 7:02 AM, Heinrich Schuchardt wrote:
>>>> Provide a unit test for the longjmp() library function
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>> v2:
>>>>  ??? no change
>>>> ---
>>>>  ? test/lib/Makefile? |? 1 +
>>>>  ? test/lib/longjmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  ? 2 files changed, 45 insertions(+)
>>>>  ? create mode 100644 test/lib/longjmp.c
>>>>
>>>> diff --git a/test/lib/Makefile b/test/lib/Makefile
>>>> index 97c11e35a8..a30f615aa9 100644
>>>> --- a/test/lib/Makefile
>>>> +++ b/test/lib/Makefile
>>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
>>>>  ? obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
>>>>  ? obj-y += hexdump.o
>>>>  ? obj-y += lmb.o
>>>> +obj-y += longjmp.o
>>>>  ? obj-$(CONFIG_CONSOLE_RECORD) += test_print.o
>>>>  ? obj-$(CONFIG_SSCANF) += sscanf.o
>>>>  ? obj-y += string.o
>>>> diff --git a/test/lib/longjmp.c b/test/lib/longjmp.c
>>>> new file mode 100644
>>>> index 0000000000..7571540ffc
>>>> --- /dev/null
>>>> +++ b/test/lib/longjmp.c
>>>> @@ -0,0 +1,44 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Test setjmp(), longjmp()
>>>> + *
>>>> + * Copyright (c) 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <test/lib.h>
>>>> +#include <test/test.h>
>>>> +#include <test/ut.h>
>>>> +#include <asm/setjmp.h>
>>>> +
>>>> +/**
>>>> + * test_longjmp_ret() - get longjmp() return value
>>>> + *
>>>> + * @i:??????? value passed to longjmp()
>>>> + * Return:??? value returned by longjmp()
>>>> + */
>>>> +int test_longjmp_ret(int i)
>>>> +{
>>>> +??? jmp_buf env;
>>>> +??? int ret;
>>>> +
>>>> +??? ret = setjmp(env);
>>>> +??? if (ret)
>>>> +??????? return ret;
>>>> +??? longjmp(env, i);
>>>> +??? /* We should not arrive here */
>>>> +??? return 0x1000;
>>>> +}
>>>> +
>>>> +static int lib_test_longjmp(struct unit_test_state *uts)
>>>> +{
>>>> +??? int i;
>>>> +
>>>> +??? for (i = -3; i < 0; ++i)
>>>> +??????? ut_asserteq(i, test_longjmp_ret(i));
>>>> +??? ut_asserteq(1, test_longjmp_ret(0));
>>>> +??? for (i = 1; i < 4; ++i)
>>>> +??????? ut_asserteq(i, test_longjmp_ret(i));
>>>> +??? return 0;
>>>> +}
>>>> +LIB_TEST(lib_test_longjmp, 0);
>>>> -- 
>>>> 2.30.2
>>>>
>>>
>>> Reviewed-by: Sean Anderson <seanga2@gmail.com>
>>> Tested-by: Sean Anderson <seanga2@gmail.com>
>>>
>>> Though I would like to test that variables are set correctly e.g. by
>>> doing
>>>
>>> int test_longjmp_ret(int i)
>>> {
>>>  ???? jmp_buf env;
>>>  ???? int ret;
>>>
>>>  ???? ret = setjmp(env);
>>>  ???? if (ret)
>>>  ???????? return ret;
>>>  ???? ret = 0x1000;
>>>  ???? longjmp(env, i);
>>>  ???? /* We should not arrive here */
>>>  ???? return ret;
>>> }
>>>
>>> --Sean
>>
>> err, rather by doing
>>
>> int test_longjmp_ret(int i)
>> {
>>  ???? jmp_buf env;
>>  ???? int ret;
>>  ???? int foo = i;
>>
>>  ???? ret = setjmp(env);
>>  ???? if (ret)
>>  ???????? return foo;
>>  ???? foo = 0x1000;
>>  ???? longjmp(env, i);
>>  ???? /* We should not arrive here */
>>  ???? return foo;
>> }
>>
>> or something else which demonstrates that variables get reset to their
>> earlier values.
>>
>> --Sean
> 
> Hello Sean,
> 
> thank you for reviewing.
> 
> Would the following make sense to you to check that the stack pointer is
> correctly restored?
> 
> 
> struct test_jmp_buf {
>          jmp_buf env;
>          int val;
> };
> 
> /**
>   * test_longjmp() - test longjmp function
>   *
>   * @i is passed to longjmp.
>   * @i << 8 is set in the environment structure.
>   *
>   * @env:        environment
>   * @i:          value passed to longjmp()
>   */
> static void noinline test_longjmp(struct test_jmp_buf *env, int i)
> {
>          env->val = i << 8;
>          longjmp(env->env, i);
> }
> 
> /**
>   * test_setjmp() - test setjmp function
>   *
>   * setjmp() will return the value @i passed to longjmp() if @i is non-zero.
>   * For @i == 0 we expect return value 1.
>   *
>   * @i << 8 will be set by test_longjmp in the environment structure.
>   * This value can be used to check that the stack frame is restored.
>   *
>   * We return the XORed values to allow simply check both at once.
>   *
>   * @i:          value passed to longjmp()
>   * Return:      values return byby longjmp()

nit: by

>   */
> static int test_setjmp(int i)
> {
>          struct test_jmp_buf env;
>          int ret;
> 
>          env.val = -1;
>          ret = setjmp(env.env);
>          if (ret)
>                  return ret ^ env.val;
>          test_longjmp(&env, i);
>          /* We should not arrive here */
>          return 0x1000;
> }
> 
> Best regards
> 
> Heinrich
> 

Yes, this looks good.

--Sean

[PATCH v2 2/2] test: unit test for longjmp

[Andreas Schwab]

On M?r 22 2021, Sean Anderson wrote:

> int test_longjmp_ret(int i)
> {
>      jmp_buf env;
>      int ret;
>      int foo = i;
>
>      ret = setjmp(env);
>      if (ret)
>          return foo;
>      foo = 0x1000;
>      longjmp(env, i);
>      /* We should not arrive here */
>      return foo;

This is undefined.  When modifying a non-volatile auto variable between
setjmp and longjmp, there is no requirement that the value is preserved.

Andreas.

-- 
Andreas Schwab, schwab at linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH v2 2/2] test: unit test for longjmp

[Heinrich Schuchardt]

On 24.03.21 10:18, Andreas Schwab wrote:
> On M?r 22 2021, Sean Anderson wrote:
>
>> int test_longjmp_ret(int i)
>> {
>>      jmp_buf env;
>>      int ret;
>>      int foo = i;
>>
>>      ret = setjmp(env);
>>      if (ret)
>>          return foo;
>>      foo = 0x1000;
>>      longjmp(env, i);
>>      /* We should not arrive here */
>>      return foo;
>
> This is undefined.  When modifying a non-volatile auto variable between
> setjmp and longjmp, there is no requirement that the value is preserved.
>
> Andreas.
>

Hello Andreas,

thank you for reviewing.

Your comment relates to the following paragraph in the C99 specification:

"All  accessible  objects  have  values,  and  all  other  components
of the  abstract  machine have  state,  as  of  the  time  the longjmp
function  was  called,  except  that  the  values  of objects  of
automatic  storage  duration  that  are  local  to  the  function
containing  the invocation  of  the  corresponding setjmp macro  that
do  not  have  volatile-qualified  type and  have  been  changed
between the setjmp invocation and longjmp call are indeterminate."

And foo is obviously "changed  between the setjmp invocation and longjmp
call".

The current version of the patch is:
https://patchwork.ozlabs.org/project/uboot/patch/20210323181127.32411-3-xypron.glpk at gmx.de/

So I guess we have to declare env as volatile in setjmp() in this
version of the patch because it is changed between the setjmp and
longjmp invocations?

Best regards

Heinrich

[PATCH v2 2/2] test: unit test for longjmp

[Andreas Schwab]

On M?r 24 2021, Heinrich Schuchardt wrote:

> And foo is obviously "changed  between the setjmp invocation and longjmp
> call".
>
> The current version of the patch is:
> https://patchwork.ozlabs.org/project/uboot/patch/20210323181127.32411-3-xypron.glpk at gmx.de/
>
> So I guess we have to declare env as volatile in setjmp() in this
> version of the patch because it is changed between the setjmp and
> longjmp invocations?

Yes, I think so, or make it static.

Andreas.

-- 
Andreas Schwab, schwab at linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH v2 2/2] test: unit test for longjmp

[Heinrich Schuchardt]

On 24.03.21 13:39, Andreas Schwab wrote:
> On M?r 24 2021, Heinrich Schuchardt wrote:
>
>> And foo is obviously "changed  between the setjmp invocation and longjmp
>> call".
>>
>> The current version of the patch is:
>> https://patchwork.ozlabs.org/project/uboot/patch/20210323181127.32411-3-xypron.glpk at gmx.de/
>>
>> So I guess we have to declare env as volatile in setjmp() in this
>> version of the patch because it is changed between the setjmp and
>> longjmp invocations?
>
> Yes, I think so, or make it static.

The whole point of this variable during test is that it lives on the
stack. So static is not what I am looking for.

Actually adding volatile does not change the generated machine code
because test_longjmp() is marked as noinline and therefore has its own
stack frame separated from test_setjmp().

But adding volatile adds to the portability. So I will respin the series.

Best regards

Heinrich