[PATCH v2 0/2] riscv: simplify longjmp
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
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
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
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
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
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
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
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() */ 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
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
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.
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-xypr...
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
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-xypr...
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.
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-xypr...
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
participants (4)
-
Andreas Schwab -
Heinrich Schuchardt -
Leo Liang -
Sean Anderson