In commit 52e423f5b93e ("tools/nolibc: export environ as a weak symbol on i386")
and friends the asm startup logic was extended to directly populate the
"environ" array.
This makes it impossible for "environ" to be dropped by the linker.
Therefore also drop the other logic to handle non-present "environ".
Signed-off-by: Thomas Weißschuh <[email protected]>
---
Note:
Given that nowadays both _auxv and environ are mandatory symbols imposed
by nolibc of pointer size does it make sense to keep the code to make
int-sized errno optional?
---
tools/include/nolibc/stdlib.h | 12 ++----------
tools/testing/selftests/nolibc/nolibc-test.c | 7 +------
2 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
index 902162f80337..bacfd35c5156 100644
--- a/tools/include/nolibc/stdlib.h
+++ b/tools/include/nolibc/stdlib.h
@@ -83,11 +83,10 @@ void free(void *ptr)
* declared as a char **, and must be terminated by a NULL (it is recommended
* to set this variable to the "envp" argument of main()). If the requested
* environment variable exists its value is returned otherwise NULL is
- * returned. getenv() is forcefully inlined so that the reference to "environ"
- * will be dropped if unused, even at -O0.
+ * returned.
*/
static __attribute__((unused))
-char *_getenv(const char *name, char **environ)
+char *getenv(const char *name)
{
int idx, i;
@@ -102,13 +101,6 @@ char *_getenv(const char *name, char **environ)
return NULL;
}
-static __inline__ __attribute__((unused,always_inline))
-char *getenv(const char *name)
-{
- extern char **environ;
- return _getenv(name, environ);
-}
-
static __attribute__((unused))
unsigned long getauxval(unsigned long type)
{
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 486334981e60..7f4611ce1795 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -40,9 +40,6 @@
#endif
#endif
-/* will be used by nolibc by getenv() */
-char **environ;
-
/* definition of a series of tests */
struct test {
const char *name; /* test name */
@@ -939,7 +936,7 @@ static const struct test test_names[] = {
{ 0 }
};
-int main(int argc, char **argv, char **envp)
+int main(int argc, char **argv)
{
int min = 0;
int max = INT_MAX;
@@ -948,8 +945,6 @@ int main(int argc, char **argv, char **envp)
int idx;
char *test;
- environ = envp;
-
/* when called as init, it's possible that no console was opened, for
* example if no /dev file system was provided. We'll check that fd#1
* was opened, and if not we'll attempt to create and open /dev/console
---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230710-nolibc-environ-79f425ce6354
Best regards,
--
Thomas Weißschuh <[email protected]>
On 2023-07-10 19:43:27+0200, Willy Tarreau wrote:
> Hi Thomas,
>
> On Mon, Jul 10, 2023 at 07:22:53PM +0200, Thomas Weißschuh wrote:
> > In commit 52e423f5b93e ("tools/nolibc: export environ as a weak symbol on i386")
> > and friends the asm startup logic was extended to directly populate the
> > "environ" array.
> >
> > This makes it impossible for "environ" to be dropped by the linker.
> > Therefore also drop the other logic to handle non-present "environ".
>
> Hmmm OK but at least I'd like that we continue to reference it from
> nolibc-test to make sure it's still visible. Maybe we could just check
> that it's always equal to envp ? If we drop its reference from there,
> sooner or later someone will find it interesting to rename it and some
> programs referencing it will break.
Easy enough to test for. I'll send a v2.
> > Note:
> >
> > Given that nowadays both _auxv and environ are mandatory symbols imposed
> > by nolibc of pointer size does it make sense to keep the code to make
> > int-sized errno optional?
>
> While it indeed used to be related to having a data segment or not
> initially, it still has an impact on our ability to completely drop
> the errno setting code from all syscalls. Given the SET_ERRNO() macro
> now I guess it's very cheap to keep it, don't you think ?
SET_ERRNO irks me a tiny bit :-)
But it's easy enough to keep, let's do so.
Just wanted to have brought it up.
Thomas
Hi Thomas,
On Mon, Jul 10, 2023 at 07:22:53PM +0200, Thomas Wei?schuh wrote:
> In commit 52e423f5b93e ("tools/nolibc: export environ as a weak symbol on i386")
> and friends the asm startup logic was extended to directly populate the
> "environ" array.
>
> This makes it impossible for "environ" to be dropped by the linker.
> Therefore also drop the other logic to handle non-present "environ".
Hmmm OK but at least I'd like that we continue to reference it from
nolibc-test to make sure it's still visible. Maybe we could just check
that it's always equal to envp ? If we drop its reference from there,
sooner or later someone will find it interesting to rename it and some
programs referencing it will break.
> Note:
>
> Given that nowadays both _auxv and environ are mandatory symbols imposed
> by nolibc of pointer size does it make sense to keep the code to make
> int-sized errno optional?
While it indeed used to be related to having a data segment or not
initially, it still has an impact on our ability to completely drop
the errno setting code from all syscalls. Given the SET_ERRNO() macro
now I guess it's very cheap to keep it, don't you think ?
Thanks,
Willy
On Mon, Jul 10, 2023 at 07:51:53PM +0200, Thomas Wei?schuh wrote:
> On 2023-07-10 19:43:27+0200, Willy Tarreau wrote:
> > Hi Thomas,
> >
> > On Mon, Jul 10, 2023 at 07:22:53PM +0200, Thomas Wei?schuh wrote:
> > > In commit 52e423f5b93e ("tools/nolibc: export environ as a weak symbol on i386")
> > > and friends the asm startup logic was extended to directly populate the
> > > "environ" array.
> > >
> > > This makes it impossible for "environ" to be dropped by the linker.
> > > Therefore also drop the other logic to handle non-present "environ".
> >
> > Hmmm OK but at least I'd like that we continue to reference it from
> > nolibc-test to make sure it's still visible. Maybe we could just check
> > that it's always equal to envp ? If we drop its reference from there,
> > sooner or later someone will find it interesting to rename it and some
> > programs referencing it will break.
>
> Easy enough to test for. I'll send a v2.
Thanks!
> > > Note:
> > >
> > > Given that nowadays both _auxv and environ are mandatory symbols imposed
> > > by nolibc of pointer size does it make sense to keep the code to make
> > > int-sized errno optional?
> >
> > While it indeed used to be related to having a data segment or not
> > initially, it still has an impact on our ability to completely drop
> > the errno setting code from all syscalls. Given the SET_ERRNO() macro
> > now I guess it's very cheap to keep it, don't you think ?
>
> SET_ERRNO irks me a tiny bit :-)
To be honest, it's the same for me. but it's cheap. And when you rebuild
a binary without it you can observe significant savings that can be
important for those who are space-constrained.
> But it's easy enough to keep, let's do so.
> Just wanted to have brought it up.
Yes, you're totally right to raise this, thank you!
Willy