[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v5] readlink: more size optimization
From:       tito <farmatito () tiscali ! it>
Date:       2023-04-25 16:22:37
Message-ID: 20230425182237.369dad0f () devuan
[Download RAW message or body]

On Tue, 25 Apr 2023 08:09:48 -0500
Eric Blake <eblake@redhat.com> wrote:

> On Tue, Apr 25, 2023 at 07:51:57AM -0500, Eric Blake wrote:
> > On Tue, Apr 25, 2023 at 10:52:45AM +1000, David Leonard wrote:
> > > On Mon, 24 Apr 2023, Harald van Dijk wrote:
> > > > coreutils/readlink.c:91:27: warning: adding 'unsigned int' to a string
> > > > does not append to the string [-Wstring-plus-int]
> > > >        printf("%s%s", buf, "\n" + (opt & 1));
> > > >                            ~~~~~^~~~~~~~~~~
> > > > coreutils/readlink.c:91:27: note: use array indexing to silence this warning
> > > >        printf("%s%s", buf, "\n" + (opt & 1));
> > > >                                 ^
> > > >                            &    [          ]
> > > > 
> > > > I am not sure what busybox's intent is here. If busybox decides that
> > > > this warning is useless, it would be nice if it got suppressed reliably.
> > > 
> > > Alternatively, avoid warnings with this construction which gives pretty
> > > good size results:
> > > 
> > > 	if (opt)
> > 
> > That's not the same as 'if (opt & 1)'.  As such, you are not comparing
> > apples to oranges...
> > 
> > >     $ cat f1.c
> > >     #include <stdio.h>
> > > 
> > >     /* suppress trailing newline when opt */
> > >     int f1(int opt, char buf[]) {
> > >     #if defined(T0)
> > > 	if (opt) printf("%s", buf); else printf("%s\n", buf);
> > >     #elif defined(T1)
> > > 	if (opt) fputs(buf, stdout); else puts(buf);
> > >     #elif defined(T2)
> > > 	printf("%s%s", buf, "\n" + (opt & 1));
> > 
> > Of the four tests, T2 is the only one with correct semantics.
> > 
> > >     #elif defined(T3)
> > > 	printf("%s%s", buf, opt ? "" : "\n");
> > >     #else
> > >     # error "forgot to -DTx"
> > >     #endif
> 
> The baseline in the existing coreutils/readlink.c is:
> 
>   printf((opt & 1) ? "%s" : "%s\n", buf);
> 
> which I don't see in your tests.
> 

Hi,
more size reduction for readlink.

Ciao,
Tito

make bloatcheck 
function                                             old     new   delta
readlink_main                                        127     118      -9
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9)               Total: -9 bytes
   text    data     bss     dec     hex filename
1007549   16467    1840 1025856   fa740 busybox_old
1007540   16467    1840 1025847   fa737 busybox_unstripped

Signed-off-by: Tito Ragusa <farmatito@tiscali.it>

--- coreutils/readlink.c.orig	2023-04-25 18:09:38.728944000 +0200
+++ coreutils/readlink.c	2023-04-25 18:10:31.160915398 +0200
@@ -68,12 +68,10 @@ int readlink_main(int argc, char **argv)
 int readlink_main(int argc UNUSED_PARAM, char **argv)
 {
 	char *buf;
-	char *fname;
 	unsigned opt;
 
 	opt = getopt32(argv, "^" "n" IF_FEATURE_READLINK_FOLLOW("fvsq")
                        "\0" "=1");
-	fname = argv[optind];
 
 	/* compat: coreutils readlink reports errors silently via exit code */
 	if (!(opt & 4)) /* not -v */
@@ -81,14 +79,18 @@ int readlink_main(int argc UNUSED_PARAM,
 
 	/* NOFORK: only one alloc is allowed; must free */
 	if (opt & 2) { /* -f */
-		buf = xmalloc_realpath_coreutils(fname);
+		buf = xmalloc_realpath_coreutils(argv[optind]);
 	} else {
-		buf = xmalloc_readlink_or_warn(fname);
+		buf = xmalloc_readlink_or_warn(argv[optind]);
 	}
 
 	if (!buf)
 		return EXIT_FAILURE;
-	printf((opt & 1) ? "%s" : "%s\n", buf);
+
+	fputs_stdout(buf);
+	if (!(opt & 1))
+		bb_putchar('\n');
+
 	free(buf);
 
 	fflush_stdout_and_exit_SUCCESS();



[Attachment #3 (text/x-patch)]

Size reduction for readlink.

make bloatcheck 
function                                             old     new   delta
readlink_main                                        127     118      -9
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9)               Total: -9 bytes
   text    data     bss     dec     hex filename
1007549   16467    1840 1025856   fa740 busybox_old
1007540   16467    1840 1025847   fa737 busybox_unstripped

Signed-off-by: Tito Ragusa <farmatito@tiscali.it>

--- coreutils/readlink.c.orig	2023-04-25 18:09:38.728944000 +0200
+++ coreutils/readlink.c	2023-04-25 18:10:31.160915398 +0200
@@ -68,12 +68,10 @@ int readlink_main(int argc, char **argv)
 int readlink_main(int argc UNUSED_PARAM, char **argv)
 {
 	char *buf;
-	char *fname;
 	unsigned opt;
 
 	opt = getopt32(argv, "^" "n" IF_FEATURE_READLINK_FOLLOW("fvsq")
                        "\0" "=1");
-	fname = argv[optind];
 
 	/* compat: coreutils readlink reports errors silently via exit code */
 	if (!(opt & 4)) /* not -v */
@@ -81,14 +79,18 @@ int readlink_main(int argc UNUSED_PARAM,
 
 	/* NOFORK: only one alloc is allowed; must free */
 	if (opt & 2) { /* -f */
-		buf = xmalloc_realpath_coreutils(fname);
+		buf = xmalloc_realpath_coreutils(argv[optind]);
 	} else {
-		buf = xmalloc_readlink_or_warn(fname);
+		buf = xmalloc_readlink_or_warn(argv[optind]);
 	}
 
 	if (!buf)
 		return EXIT_FAILURE;
-	printf((opt & 1) ? "%s" : "%s\n", buf);
+
+	fputs_stdout(buf);
+	if (!(opt & 1))
+		bb_putchar('\n');
+
 	free(buf);
 
 	fflush_stdout_and_exit_SUCCESS();


_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


[prev in list] [next in list] [prev in thread] [next in thread]