Re: rm(1) static addition
On 28 April 2013 15:25, Marc Espie es...@nerim.net wrote: On Sat, Apr 27, 2013 at 09:12:21AM -0400, Eitan Adler wrote: FWIW I don't believe this sort of patch significantly affects debugging because that should be done with -O0 -g anyways. Bwahahaha You're lucky to not run into compiler bugs that only show up with -O2. Good luck figuring THOSE out with -O0. I have run into those bugs, and I have debugged them before: I've found both application and compiler bugs like this. :) -- Eitan Adler
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote:
Hey all,
Time for attempt #2!
Adding static to internal function allows the compiler to better
detect dead code (functions, variables, etc) and makes it easier for
the compiler to optimize; e.g., since it knows a function will only
called once it can inline code; or not output a symbol for a certain
function.
In general we don't lik this because it makes things harder to debug.
For libraries, yes, but for programs, no.
-Otto
I'm offering this patch for review:
Index: rm.c
===
RCS file: /cvs/src/bin/rm/rm.c,v
retrieving revision 1.27
diff -u -r1.27 rm.c
--- rm.c 5 Sep 2012 19:49:08 - 1.27
+++ rm.c 27 Apr 2013 04:26:18 -
@@ -49,15 +49,15 @@
extern char *__progname;
-int dflag, eval, fflag, iflag, Pflag, stdin_ok;
+static int dflag, eval, fflag, iflag, Pflag, stdin_ok;
-int check(char *, char *, struct stat *);
-void checkdot(char **);
-void rm_file(char **);
-int rm_overwrite(char *, struct stat *);
-int pass(int, off_t, char *, size_t);
-void rm_tree(char **);
-void usage(void);
+static int check(char *, char *, struct stat *);
+static void checkdot(char **);
+static void rm_file(char **);
+static int rm_overwrite(char *, struct stat *);
+static int pass(int, off_t, char *, size_t);
+static void rm_tree(char **);
+static void usage(void);
/*
* rm --
@@ -117,7 +117,7 @@
exit (eval);
}
-void
+static void
rm_tree(char **argv)
{
FTS *fts;
@@ -217,7 +217,7 @@
fts_close(fts);
}
-void
+static void
rm_file(char **argv)
{
struct stat sb;
@@ -271,7 +271,7 @@
* kernel support.
* Returns 1 for success.
*/
-int
+static int
rm_overwrite(char *file, struct stat *sbp)
{
struct stat sb, sb2;
@@ -324,7 +324,7 @@
return (0);
}
-int
+static int
pass(int fd, off_t len, char *buf, size_t bsize)
{
size_t wlen;
@@ -338,7 +338,7 @@
return (1);
}
-int
+static int
check(char *path, char *name, struct stat *sp)
{
int ch, first;
@@ -380,7 +380,7 @@
* trailing slashes have been removed, we'll remove them here.
*/
#define ISDOT(a) ((a)[0] == '.' (!(a)[1] || ((a)[1] == '.'
!(a)[2])))
-void
+static void
checkdot(char **argv)
{
char *p, **save, **t;
@@ -411,7 +411,7 @@
}
}
-void
+static void
usage(void)
{
(void)fprintf(stderr, usage: %s [-dfiPRr] file ...\n, __progname);
--
Eitan Adler
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 09:12:21AM -0400, Eitan Adler wrote: On 27 April 2013 09:06, Kenneth R Westerback kwesterb...@rogers.com wrote: On Sat, Apr 27, 2013 at 08:10:41AM +0200, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Hey all, Time for attempt #2! Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. -Otto +1. We see way more 'nuke stupid static crap' diffs that 'add static' diffs. We are even dubious about almost all inline functions since they are also harder to debug and (on most 'modern' archs) add little if any performance but do make executables bigger. Just in case you have a 'use inline functions to speed things up just like XBSD' diff in the queue, and were about to hit another sensitive button issue. :-) Most of my diffs are take recent^W changes from the other BSDs if they are useful. FWIW I don't believe this sort of patch significantly affects debugging because that should be done with -O0 -g anyways. Odd how often people running release, and who don't want to compile shit, report problems we'd like them to be able to provide more info on. :-) Ken That said, thanks for the info. If I have other diffs which are more suitable to OpenBSD I'll be sure to send them. Most the remainder are similar cleanup or non-POSIX feature-adds. -- Eitan Adler
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 08:10:41AM +0200, Otto Moerbeek wrote:
On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote:
Hey all,
Time for attempt #2!
Adding static to internal function allows the compiler to better
detect dead code (functions, variables, etc) and makes it easier for
the compiler to optimize; e.g., since it knows a function will only
called once it can inline code; or not output a symbol for a certain
function.
In general we don't lik this because it makes things harder to debug.
For libraries, yes, but for programs, no.
-Otto
+1. We see way more 'nuke stupid static crap' diffs that 'add static'
diffs. We are even dubious about almost all inline functions since
they are also harder to debug and (on most 'modern' archs) add
little if any performance but do make executables bigger. Just in
case you have a 'use inline functions to speed things up just like
XBSD' diff in the queue, and were about to hit another sensitive
button issue. :-)
Ken
I'm offering this patch for review:
Index: rm.c
===
RCS file: /cvs/src/bin/rm/rm.c,v
retrieving revision 1.27
diff -u -r1.27 rm.c
--- rm.c5 Sep 2012 19:49:08 - 1.27
+++ rm.c27 Apr 2013 04:26:18 -
@@ -49,15 +49,15 @@
extern char *__progname;
-int dflag, eval, fflag, iflag, Pflag, stdin_ok;
+static int dflag, eval, fflag, iflag, Pflag, stdin_ok;
-intcheck(char *, char *, struct stat *);
-void checkdot(char **);
-void rm_file(char **);
-intrm_overwrite(char *, struct stat *);
-intpass(int, off_t, char *, size_t);
-void rm_tree(char **);
-void usage(void);
+static int check(char *, char *, struct stat *);
+static voidcheckdot(char **);
+static voidrm_file(char **);
+static int rm_overwrite(char *, struct stat *);
+static int pass(int, off_t, char *, size_t);
+static voidrm_tree(char **);
+static voidusage(void);
/*
* rm --
@@ -117,7 +117,7 @@
exit (eval);
}
-void
+static void
rm_tree(char **argv)
{
FTS *fts;
@@ -217,7 +217,7 @@
fts_close(fts);
}
-void
+static void
rm_file(char **argv)
{
struct stat sb;
@@ -271,7 +271,7 @@
* kernel support.
* Returns 1 for success.
*/
-int
+static int
rm_overwrite(char *file, struct stat *sbp)
{
struct stat sb, sb2;
@@ -324,7 +324,7 @@
return (0);
}
-int
+static int
pass(int fd, off_t len, char *buf, size_t bsize)
{
size_t wlen;
@@ -338,7 +338,7 @@
return (1);
}
-int
+static int
check(char *path, char *name, struct stat *sp)
{
int ch, first;
@@ -380,7 +380,7 @@
* trailing slashes have been removed, we'll remove them here.
*/
#define ISDOT(a) ((a)[0] == '.' (!(a)[1] || ((a)[1] == '.'
!(a)[2])))
-void
+static void
checkdot(char **argv)
{
char *p, **save, **t;
@@ -411,7 +411,7 @@
}
}
-void
+static void
usage(void)
{
(void)fprintf(stderr, usage: %s [-dfiPRr] file ...\n, __progname);
--
Eitan Adler
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. Isn't that rule only for the kernel? ddb can only see global symbols, but gdb should work fine in userland. Certainly I can set breakpoints on static functions, even when compiled without -g. I think the diff is fine.
Re: rm(1) static addition
On Apr 27, 2013, at 7:36 PM, Ted Unangst t...@tedunangst.com wrote: On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. Isn't that rule only for the kernel? ddb can only see global symbols, but gdb should work fine in userland. Certainly I can set breakpoints on static functions, even when compiled without -g. On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? Thanks, Franco
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 09:09:25PM +0200, Franco Fichtner wrote: On Apr 27, 2013, at 7:36 PM, Ted Unangst t...@tedunangst.com wrote: On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. Isn't that rule only for the kernel? ddb can only see global symbols, but gdb should work fine in userland. Certainly I can set breakpoints on static functions, even when compiled without -g. On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? Inlined functions are pretyy confusing in gdb. -Otto
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 09:09:25PM +0200, Franco Fichtner wrote: On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? That's not true in general. backtrace(3) or any other user of the .eh_frame section for unwinding won't see individual records for inlined functions. Static functions are more likely to get inlined, but the same can happen with non-inline functions defined in the same file. Joerg
Re: rm(1) static addition
On Apr 27, 2013, at 9:28 PM, Joerg Sonnenberger jo...@britannica.bec.de wrote: On Sat, Apr 27, 2013 at 09:09:25PM +0200, Franco Fichtner wrote: On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? That's not true in general. backtrace(3) or any other user of the .eh_frame section for unwinding won't see individual records for inlined functions. Static functions are more likely to get inlined, but the same can happen with non-inline functions defined in the same file. Ok, thanks. Sounds like an impasse. GCC does what it wants, but then -fno-inline is also not an option for the build? Regards, Franco
Re: rm(1) static addition
On Sat, Apr 27, 2013 at 09:14:59PM +0200, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 09:09:25PM +0200, Franco Fichtner wrote: On Apr 27, 2013, at 7:36 PM, Ted Unangst t...@tedunangst.com wrote: On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. Isn't that rule only for the kernel? ddb can only see global symbols, but gdb should work fine in userland. Certainly I can set breakpoints on static functions, even when compiled without -g. On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? Inlined functions are pretyy confusing in gdb. So basically we're making the code more difficult to understand because we don't want to spend the one-time effort to move the static inlinification from -O2 to -O3... I'm not convinced we should give up that easily ;) It should even be possible to have static functions including breakpoints in the kernel. There is nothing particularly odd about them as long as they are not inlined. There may be more than one with the same name but the address-name mapping is distinct, isn't it? -Otto
Re: rm(1) static addition
Date: Sat, 27 Apr 2013 13:36:31 -0400 From: Ted Unangst t...@tedunangst.com On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. Isn't that rule only for the kernel? ddb can only see global symbols, but gdb should work fine in userland. Certainly I can set breakpoints on static functions, even when compiled without -g. That means you didn't test well enough ;).
Re: rm(1) static addition
On 27 April 2013 15:38, Tobias Ulmer tobi...@tmux.org wrote: On Sat, Apr 27, 2013 at 09:14:59PM +0200, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 09:09:25PM +0200, Franco Fichtner wrote: On Apr 27, 2013, at 7:36 PM, Ted Unangst t...@tedunangst.com wrote: On Sat, Apr 27, 2013 at 08:10, Otto Moerbeek wrote: On Sat, Apr 27, 2013 at 01:08:06AM -0400, Eitan Adler wrote: Adding static to internal function allows the compiler to better detect dead code (functions, variables, etc) and makes it easier for the compiler to optimize; e.g., since it knows a function will only called once it can inline code; or not output a symbol for a certain function. In general we don't lik this because it makes things harder to debug. For libraries, yes, but for programs, no. Isn't that rule only for the kernel? ddb can only see global symbols, but gdb should work fine in userland. Certainly I can set breakpoints on static functions, even when compiled without -g. On backtrace(3) (which is a GNU thing, I know), static functions don't show up with their respective names even though they are in the binary. That's a tad annoying, but I am not aware of any other limitation. Can someone please enlighten me? Inlined functions are pretyy confusing in gdb. So basically we're making the code more difficult to understand because we don't want to spend the one-time effort to move the static inlinification from -O2 to -O3... -finline-functions is enabled at -O3 only. -finline-functions-called-once is enabled at even -O1 -- Eitan Adler
rm(1) static addition
Hey all,
Time for attempt #2!
Adding static to internal function allows the compiler to better
detect dead code (functions, variables, etc) and makes it easier for
the compiler to optimize; e.g., since it knows a function will only
called once it can inline code; or not output a symbol for a certain
function.
I'm offering this patch for review:
Index: rm.c
===
RCS file: /cvs/src/bin/rm/rm.c,v
retrieving revision 1.27
diff -u -r1.27 rm.c
--- rm.c5 Sep 2012 19:49:08 - 1.27
+++ rm.c27 Apr 2013 04:26:18 -
@@ -49,15 +49,15 @@
extern char *__progname;
-int dflag, eval, fflag, iflag, Pflag, stdin_ok;
+static int dflag, eval, fflag, iflag, Pflag, stdin_ok;
-intcheck(char *, char *, struct stat *);
-void checkdot(char **);
-void rm_file(char **);
-intrm_overwrite(char *, struct stat *);
-intpass(int, off_t, char *, size_t);
-void rm_tree(char **);
-void usage(void);
+static int check(char *, char *, struct stat *);
+static voidcheckdot(char **);
+static voidrm_file(char **);
+static int rm_overwrite(char *, struct stat *);
+static int pass(int, off_t, char *, size_t);
+static voidrm_tree(char **);
+static voidusage(void);
/*
* rm --
@@ -117,7 +117,7 @@
exit (eval);
}
-void
+static void
rm_tree(char **argv)
{
FTS *fts;
@@ -217,7 +217,7 @@
fts_close(fts);
}
-void
+static void
rm_file(char **argv)
{
struct stat sb;
@@ -271,7 +271,7 @@
* kernel support.
* Returns 1 for success.
*/
-int
+static int
rm_overwrite(char *file, struct stat *sbp)
{
struct stat sb, sb2;
@@ -324,7 +324,7 @@
return (0);
}
-int
+static int
pass(int fd, off_t len, char *buf, size_t bsize)
{
size_t wlen;
@@ -338,7 +338,7 @@
return (1);
}
-int
+static int
check(char *path, char *name, struct stat *sp)
{
int ch, first;
@@ -380,7 +380,7 @@
* trailing slashes have been removed, we'll remove them here.
*/
#define ISDOT(a) ((a)[0] == '.' (!(a)[1] || ((a)[1] == '.'
!(a)[2])))
-void
+static void
checkdot(char **argv)
{
char *p, **save, **t;
@@ -411,7 +411,7 @@
}
}
-void
+static void
usage(void)
{
(void)fprintf(stderr, usage: %s [-dfiPRr] file ...\n, __progname);
--
Eitan Adler
