[PATCH] Re: glitch with strlen [was: Re: Shells in 1.1.2]

[Bernhard Fischer]

On Fri, May 05, 2006 at 09:40:56AM +0200, Bernhard Fischer wrote:
>On Thu, May 04, 2006 at 05:24:58PM -0400, Rob Landley wrote:
>>On Thursday 04 May 2006 2:25 pm, Bernhard Fischer wrote:
>>> On Thu, May 04, 2006 at 07:54:28PM +0200, Bernhard Fischer wrote:
>>> >On Thu, May 04, 2006 at 01:02:43PM -0400, Joe Shaw wrote:
>>> >>ok, the default config worked fine, so i guess it's my config...i will
>>> >>put below
>>> >>i'm using gcc 4.1
>>> >>my config will not work with either of the 4 shells
>>> >>running tty gave me "/dev/pts/1"
>>> >>i just realized i'm talking to the rob landley that wrote "How to use
>>> >>initramfs"
>>> >>thank you for that article sir, very useful
>>> >>
>>> >>ok, now my config
>>> >>joe
>>> >
>>> >Sounds like
>>> >http://busybox.net/bugs/view.php?id=847
>>> >
>>> >Please confirm if the patch there makes it behave for you, conceptually.
>>>
>>> The patch does fix it for me, without any size changes for the legacy
>>> mode of compiling the source files one-by-one.
>>>
>>> I'm not entirely sure how to fix this in a compiler agnostic way unless
>>> i do away with the #undef and adjust all users to use bb_strlen
>>> directly instead of relying on the preprocessor to fix the affected
>>> applets up.
>>>
>>> Thoughts?
>>
>>We only need bb_strlen() on gcc anyway, since other compilers may not force 
>>inline strlen() in the first place.  But we're not compiler agnostic, and 
>>last I checked we weren't even pretending to be.  (Platform, yes.  Compiler, 
>>no.)
>
>I don't know, but i try to write C code for busybox. YMMV :P
>
>>Sigh.  bb_strlen() always struck me as a marginal optimization.  The 
>>complexity is _almost_ worth it, but it keeps breaking.  I remember 
>>complaining when the #undef plus #include "libbb.h" in the middle of the file 
>>went in.  UGLY...
>>
>>Alright, I just checked in a fix.  It's not a non-ugly fix, but it's at least 
>>less ugly than what was there.
>
>This is not a fix but breaks the compilation for non-gcc compiles.
>
>Can we _discuss_ how to fix this properly?
>
>
>This optimization helps at least with gcc which is what most people will
>use, probably.
>I suggested above that we don't rely on the preprocessor but fix the
>applets proper to use bb_strlen. This would IMO be a clean solution
>which is safe and trivial.
>
>Thoughts?

Proposed real fix:
apply attached busybox.fix_strlen.01.diff
and then replace all strlen() with bb_strlen() e.g. via
find ./ -name "*.c" -exec egrep --exclude='*xfuncs.c*' \
--exclude='*scripts/*' -Hl "[^_]strlen" {} \; | xargs \
sed -i -e "/[^_]strlen/s/strlen/bb_strlen/g"

The end result is that it is even smaller for the .config provided by
Joe Shaw earlier in this thread and does what it's supposed to do:

   text    data     bss     dec     hex filename
 867078    7656  825676 1700410  19f23a ../bb.svn.oorig/busybox.landley
 867062    7656  825676 1700394  19f22a busybox.strlen_ok.gcc-4.1

objections or can i go and check this in early next week?
>
>>
>>I need to look through the bug generator and close some of these out...
>>
>>Rob
>>-- 
>>Never bet against the cheap plastic solution.
>>
>_______________________________________________
>busybox mailing list
>busybox at busybox.net
>http://busybox.net/cgi-bin/mailman/listinfo/busybox
>
-------------- next part --------------
Index: include/libbb.h
===================================================================
--- include/libbb.h	(revision 15002)
+++ include/libbb.h	(working copy)
@@ -414,6 +414,8 @@ int is_in_ino_dev_hashtable(const struct
 void add_to_ino_dev_hashtable(const struct stat *statbuf, const char *name);
 void reset_ino_dev_hashtable(void);
 
+extern size_t bb_strlen(const char *string);
+
 char *bb_xasprintf(const char *format, ...) __attribute__ ((format (printf, 1, 2)));
 
 #define FAIL_DELAY    3
Index: include/platform.h
===================================================================
--- include/platform.h	(revision 15002)
+++ include/platform.h	(working copy)
@@ -67,10 +67,13 @@
 # endif
 #endif
 
-#ifdef __GNUC__
-#define strlen(x) bb_strlen(x)
-extern size_t bb_strlen(const char *string);
-#endif
+/* ---- Compiler dependent settings ------------------------- */
+#ifndef __GNUC__
+#if defined __INTEL_COMPILER
+__extension__ typedef __signed__ long long __s64;
+__extension__ typedef unsigned long long __u64;
+#endif /* __INTEL_COMPILER */
+#endif /* ifndef __GNUC__ */
 
 /* ---- Endian Detection ------------------------------------ */
 #ifndef __APPLE__
Index: libbb/xfuncs.c
===================================================================
--- libbb/xfuncs.c	(revision 15002)
+++ libbb/xfuncs.c	(working copy)
@@ -174,12 +174,13 @@ void bb_xfflush_stdout(void)
 }
 #endif
 
-// GCC forces inlining of strlen everywhere, which is generally a byte
-// larger than calling a function, and it's called a lot so it adds up.
+/* Provide 1 possibly inlined call to strlen().
+ * Calling a function is smaller than possibly inlining the impl everywhere.
+ */
 #ifdef L_strlen
 size_t bb_strlen(const char *string)
 {
-	    return(__builtin_strlen(string));
+	    return (strlen(string));
 }
 #endif