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
[ntpd] Useless variables...
Hi, here is a patch to remove two useless variables in ntpd. Spotted when recompiling with -Wextra. Ok/Comments? Index: client.c === RCS file: /cvs/src/usr.sbin/ntpd/client.c,v retrieving revision 1.89 diff -u -p -r1.89 client.c --- client.c21 Sep 2011 15:41:30 - 1.89 +++ client.c27 Apr 2013 21:59:52 - @@ -186,8 +186,7 @@ client_query(struct ntp_peer *p) p-query-msg.xmttime.fractionl = arc4random(); p-query-xmttime = gettime_corrected(); - if (ntp_sendmsg(p-query-fd, NULL, p-query-msg, - NTP_MSGSIZE_NOAUTH, 0) == -1) { + if (ntp_sendmsg(p-query-fd, NULL, p-query-msg) == -1) { p-senderrors++; set_next(p, INTERVAL_QUERY_PATHETIC); p-trustlevel = TRUSTLEVEL_PATHETIC; Index: ntp_msg.c === RCS file: /cvs/src/usr.sbin/ntpd/ntp_msg.c,v retrieving revision 1.18 diff -u -p -r1.18 ntp_msg.c --- ntp_msg.c 19 Oct 2007 15:53:57 - 1.18 +++ ntp_msg.c 27 Apr 2013 21:59:52 - @@ -40,8 +40,7 @@ ntp_getmsg(struct sockaddr *sa, char *p, } int -ntp_sendmsg(int fd, struct sockaddr *sa, struct ntp_msg *msg, ssize_t len, -int auth) +ntp_sendmsg(int fd, struct sockaddr *sa, struct ntp_msg *msg) { socklen_t sa_len; ssize_t n; Index: ntpd.h === RCS file: /cvs/src/usr.sbin/ntpd/ntpd.h,v retrieving revision 1.106 diff -u -p -r1.106 ntpd.h --- ntpd.h 20 Sep 2012 12:43:16 - 1.106 +++ ntpd.h 27 Apr 2013 21:59:52 - @@ -226,7 +226,7 @@ struct ntp_conf_sensor *new_sensor(char /* ntp_msg.c */ intntp_getmsg(struct sockaddr *, char *, ssize_t, struct ntp_msg *); -intntp_sendmsg(int, struct sockaddr *, struct ntp_msg *, ssize_t, int); +intntp_sendmsg(int, struct sockaddr *, struct ntp_msg *); /* server.c */ intsetup_listeners(struct servent *, struct ntpd_conf *, u_int *); Index: server.c === RCS file: /cvs/src/usr.sbin/ntpd/server.c,v retrieving revision 1.36 diff -u -p -r1.36 server.c --- server.c21 Sep 2011 15:41:30 - 1.36 +++ server.c27 Apr 2013 21:59:52 - @@ -210,6 +210,6 @@ server_dispatch(int fd, struct ntpd_conf reply.rootdelay = d_to_sfp(lconf-status.rootdelay); reply.refid = lconf-status.refid; - ntp_sendmsg(fd, (struct sockaddr *)fsa, reply, size, 0); + ntp_sendmsg(fd, (struct sockaddr *)fsa, reply); return (0); }
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: [ntpd] Useless variables...
On Sat, Apr 27, 2013 at 10:30:01PM +0200, Maxime Villard wrote: Hi, here is a patch to remove two useless variables in ntpd. Spotted when recompiling with -Wextra. Ok/Comments? Looks like 'auth' has been removed in revision 1.8 and 'len' in revision 1.17 of ntp_msg.c but not removed from the function arguments list. So I think the diff is good. Index: client.c === RCS file: /cvs/src/usr.sbin/ntpd/client.c,v retrieving revision 1.89 diff -u -p -r1.89 client.c --- client.c 21 Sep 2011 15:41:30 - 1.89 +++ client.c 27 Apr 2013 21:59:52 - @@ -186,8 +186,7 @@ client_query(struct ntp_peer *p) p-query-msg.xmttime.fractionl = arc4random(); p-query-xmttime = gettime_corrected(); - if (ntp_sendmsg(p-query-fd, NULL, p-query-msg, - NTP_MSGSIZE_NOAUTH, 0) == -1) { + if (ntp_sendmsg(p-query-fd, NULL, p-query-msg) == -1) { p-senderrors++; set_next(p, INTERVAL_QUERY_PATHETIC); p-trustlevel = TRUSTLEVEL_PATHETIC; Index: ntp_msg.c === RCS file: /cvs/src/usr.sbin/ntpd/ntp_msg.c,v retrieving revision 1.18 diff -u -p -r1.18 ntp_msg.c --- ntp_msg.c 19 Oct 2007 15:53:57 - 1.18 +++ ntp_msg.c 27 Apr 2013 21:59:52 - @@ -40,8 +40,7 @@ ntp_getmsg(struct sockaddr *sa, char *p, } int -ntp_sendmsg(int fd, struct sockaddr *sa, struct ntp_msg *msg, ssize_t len, -int auth) +ntp_sendmsg(int fd, struct sockaddr *sa, struct ntp_msg *msg) { socklen_t sa_len; ssize_t n; Index: ntpd.h === RCS file: /cvs/src/usr.sbin/ntpd/ntpd.h,v retrieving revision 1.106 diff -u -p -r1.106 ntpd.h --- ntpd.h20 Sep 2012 12:43:16 - 1.106 +++ ntpd.h27 Apr 2013 21:59:52 - @@ -226,7 +226,7 @@ struct ntp_conf_sensor*new_sensor(char /* ntp_msg.c */ int ntp_getmsg(struct sockaddr *, char *, ssize_t, struct ntp_msg *); -int ntp_sendmsg(int, struct sockaddr *, struct ntp_msg *, ssize_t, int); +int ntp_sendmsg(int, struct sockaddr *, struct ntp_msg *); /* server.c */ int setup_listeners(struct servent *, struct ntpd_conf *, u_int *); Index: server.c === RCS file: /cvs/src/usr.sbin/ntpd/server.c,v retrieving revision 1.36 diff -u -p -r1.36 server.c --- server.c 21 Sep 2011 15:41:30 - 1.36 +++ server.c 27 Apr 2013 21:59:52 - @@ -210,6 +210,6 @@ server_dispatch(int fd, struct ntpd_conf reply.rootdelay = d_to_sfp(lconf-status.rootdelay); reply.refid = lconf-status.refid; - ntp_sendmsg(fd, (struct sockaddr *)fsa, reply, size, 0); + ntp_sendmsg(fd, (struct sockaddr *)fsa, reply); return (0); }
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