On 2023-07-14 13:58:13+0800, Zhangjin Wu wrote:
> [..]
> Which one do you prefer? the one with local variables may be more readable (not
> that much), the one with global variables has smaller text size (and therefore
> smaller memory footprint).
The one with local variables. But not by much.
> BTW, just found an arch-<ARCH>.h bug with -O0, seems the
> 'optimize("omit-frame-pointer")' attribute not really work as expected with
> -O0. It uses frame pointer for _start eventually and breaks the stack pointer
> variable (a push 'rbp' inserted at the begging of _start, so, the real rsp has
> an offset), so, therefore, it is not able to get the right argc, argv, environ
> and _auxv with -O0 currently. A solution is reverting _start to pure assembly.
>
> For the above tests, I manually reverted the arch-x86_64.h to:
>
> __asm__(
> ".text\n"
> ".weak _start\n"
> "_start:\n"
> #ifdef _NOLIBC_STACKPROTECTOR
> "call __stack_chk_init\n" /* initialize stack protector */
> #endif
> "xor %ebp, %ebp\n" /* zero the stack frame */
> "mov %rsp, %rdi\n" /* save stack pointer to %rdi, as arg1 of _start_c */
> "and $-16, %rsp\n" /* %rsp must be 16-byte aligned before call */
> "call _start_c\n" /* transfer to c runtime */
> "hlt\n" /* ensure it does not return */
> );
>
>
> 'man gcc' shows:
>
> Most optimizations are completely disabled at -O0 or if an -O level is not set on the command line, even if individual optimization flags are specified.
>
> To want -O0 work again, since now we have C _start_c, is it ok for us to revert
> the commit 7f8548589661 ("tools/nolibc: make compiler and assembler agree on
> the section around _start") and the later __no_stack_protector changes?
This commit explicitly mentions being tested with -O0 on x86_64.
I was also not able to reproduce the issue.
Before doing any reverts I think some more investigation is in order.
Can you provide exact reproduction steps?
> At the same time, to verify such issues, as Thomas suggested, in this series,
> we may need to add more startup tests to verify argc, argv, environ, _auxv, do
> we need to add a standalone run_startup (or run_crt) test entry just like
> run_syscall? or, let's simply add more in the run_stdlib, like the environ test
> added by Thomas. seems, the argc test is necessary one currently missing (argc
> >= 1):
>
> CASE_TEST(argc); EXPECT_GE(1, test_argc, 1); break;
>
> As we summarized before, the related test cases are:
>
> argv0:
>
> CASE_TEST(chmod_argv0); EXPECT_SYSZR(1, chmod(test_argv0, 0555)); break;
> CASE_TEST(chroot_exe); EXPECT_SYSER(1, chroot(test_argv0), -1, ENOTDIR); break;
>
> environ:
>
> CASE_TEST(chdir_root); EXPECT_SYSZR(1, chdir("/")); chdir(getenv("PWD")); break;
> CASE_TEST(environ); EXPECT_PTREQ(1, environ, test_envp); break;
> CASE_TEST(getenv_TERM); EXPECT_STRNZ(1, getenv("TERM")); break;
> CASE_TEST(getenv_blah); EXPECT_STRZR(1, getenv("blah")); break;
>
> auxv:
>
> CASE_TEST(getpagesize); EXPECT_SYSZR(1, test_getpagesize()); break;
>
> The above tests are in different test group and are not aimed to startup test,
> we'd better add a run_startup (or run_crt) test group before any other tests,
> it is a requiremnt of the others, we at least have these ones:
>
> +int run_startup(int min, int max)
> +{
> + int test;
> + int tmp;
> + int ret = 0;
> +
> + for (test = min; test >= 0 && test <= max; test++) {
> + int llen = 0; /* line length */
> +
> + /* avoid leaving empty lines below, this will insert holes into
> + * test numbers.
> + */
> + switch (test + __LINE__ + 1) {
> + CASE_TEST(argc); EXPECT_GE(1, test_argc, 1); break;
> + CASE_TEST(argv_addr); EXPECT_PTRNZ(1, test_argv); break;
> + CASE_TEST(argv_total); EXPECT_EQ(1, environ - test_argv - 1, test_argc); break;
> + CASE_TEST(argv0_addr); EXPECT_PTRNZ(1, argv0); break;
> + CASE_TEST(argv0_str); EXPECT_STRNZ(1, argv0); break;
> + CASE_TEST(argv0_len); EXPECT_GE(1, strlen(argv0), 1); break;
> + CASE_TEST(environ_addr); EXPECT_PTRNZ(1, environ); break;
> + CASE_TEST(environ_envp); EXPECT_PTREQ(1, environ, test_envp); break;
> + CASE_TEST(environ_total); EXPECT_GE(1, (void *)_auxv - (void *)environ - 1, 1); break;
> + CASE_TEST(_auxv_addr); EXPECT_PTRNZ(1, _auxv); break;
> + case __LINE__:
> + return ret; /* must be last */
> + /* note: do not set any defaults so as to permit holes above */
> + }
> + }
> + return ret;
> +}
>
> Any more?
My original idea was to have tests that exec /proc/self/exe or argv0.
This way we can actually pass and validate arbitrary argc, argv and
environ values.
But looking at your list, that should be enough.
> [..]
