There are more than 5000 warnings issued during "make buildworld". Some of them are false positives, but some of them are sign of true errors. Nobody is upset by warnings due it's amount, so some errors remain uncorrected. I want to cleanup the code-base from warnings, so warnings will become "attention mark" again. usr.sbin/rpcbind/rpcbind.c:239: warning: 'fd' might be used uninitialized in this function usr.sbin/rpcbind/rpcbind.c:243: warning: 'my_xprt' might be used uninitialized in this function both variables are initialized before use with no exception How-To-Repeat: N/A
On 2004-09-12 04:38, Dan Lukes <dan@obluda.cz> wrote: > static int > init_transport(struct netconfig *nconf) > { > - int fd; > + int fd = fd; /* init to suppres "may be used uninitialized" warning */ Err, no please. GCC complains that fd "may be used uninitialized" because of this construct near line 275 and the rest of the function body: 275 if (nconf->nc_semantics != NC_TPI_CLTS) { 276 if ((fd = __rpc_nconf2fd(nconf)) < 0) { 277 int non_fatal = 0; ... 283 return (1); 284 } ... 311 if (nconf->nc_semantics == NC_TPI_CLTS) { ... 334 while (nhostsbak > 0) { ... 339 if ((fd = __rpc_nconf2fd(nconf)) < 0) { ... 452 } else { ... 466 if (bind(fd, sa, addrlen) < 0) { Pay attention to this last line. GCC believes that fd can be used uninitialized because if NC_TPI_CLTS is not used the bind() call will receive an uninitialized value of `fd'. I'm not acquainted with the internals of rpcbind() at all, but I know that Alfred Perlstein is. He's probably the best person to suggest a fix for this warning, if one is really needed. > struct t_bind taddr; > struct addrinfo hints, *res = NULL; > struct __rpc_sockinfo si; > - SVCXPRT *my_xprt; > ! SVCXPRT *my_xprt = my_xprt; /* init to suppres "may be used uninitialized" warning */ No. I don't know why you think that this is a good fix for all the uninitialized pointer warnings. It's not. Never :-/ This warning is probably caused by GCC's inability to make sure that when `bar' is a pointer this construct is perfectly fine: if (foo) { ... if (bar = baz()) ... } else { ... if (bar = baz2()) ... } if (blah(bar)) ... After a quick glance at the source of rpcbind() I think that there's no way that my_xrpt can be used uninitialized. A proper fix for this warning would be then to just set my_xrpt to NULL at first and let the rest of the function unchanged regarding my_xrpt.
I'm having a hard time seeing these warnings, how are they being triggered? * Giorgos Keramidas <keramida@freebsd.org> [040912 13:07] wrote: > On 2004-09-12 04:38, Dan Lukes <dan@obluda.cz> wrote: > > static int > > init_transport(struct netconfig *nconf) > > { > > - int fd; > > + int fd = fd; /* init to suppres "may be used uninitialized" warning */ > > Err, no please. GCC complains that fd "may be used uninitialized" because > of this construct near line 275 and the rest of the function body: > > 275 if (nconf->nc_semantics != NC_TPI_CLTS) { > 276 if ((fd = __rpc_nconf2fd(nconf)) < 0) { > 277 int non_fatal = 0; > ... > 283 return (1); > 284 } > ... > 311 if (nconf->nc_semantics == NC_TPI_CLTS) { > ... > 334 while (nhostsbak > 0) { > ... > 339 if ((fd = __rpc_nconf2fd(nconf)) < 0) { > ... > 452 } else { > ... > 466 if (bind(fd, sa, addrlen) < 0) { > > Pay attention to this last line. GCC believes that fd can be used > uninitialized because if NC_TPI_CLTS is not used the bind() call will > receive an uninitialized value of `fd'. I'm not acquainted with the > internals of rpcbind() at all, but I know that Alfred Perlstein is. > He's probably the best person to suggest a fix for this warning, if one > is really needed. > > > struct t_bind taddr; > > struct addrinfo hints, *res = NULL; > > struct __rpc_sockinfo si; > > - SVCXPRT *my_xprt; > > ! SVCXPRT *my_xprt = my_xprt; /* init to suppres "may be used uninitialized" warning */ > > No. I don't know why you think that this is a good fix for all the > uninitialized pointer warnings. It's not. Never :-/ > > This warning is probably caused by GCC's inability to make sure that when > `bar' is a pointer this construct is perfectly fine: > > if (foo) { > ... > if (bar = baz()) > ... > } else { > ... > if (bar = baz2()) > ... > } > > if (blah(bar)) > ... > > After a quick glance at the source of rpcbind() I think that there's no way > that my_xrpt can be used uninitialized. A proper fix for this warning > would be then to just set my_xrpt to NULL at first and let the rest of the > function unchanged regarding my_xrpt. -- - Alfred Perlstein - Research Engineering Development Inc. - email: bright@mu.org cell: 408-480-4684
On 2004-09-12 13:35, Alfred Perlstein <alfred@freebsd.org> wrote: > I'm having a hard time seeing these warnings, how are they > being triggered? I tried building rpcbind with WARNS=3 or higher (up to 5).
* Giorgos Keramidas <keramida@freebsd.org> [040912 15:27] wrote: > On 2004-09-12 13:35, Alfred Perlstein <alfred@freebsd.org> wrote: > > I'm having a hard time seeing these warnings, how are they > > being triggered? > > I tried building rpcbind with WARNS=3 or higher (up to 5). I don't see unitialized variables, I see bad casts. -- - Alfred Perlstein - Research Engineering Development Inc. - email: bright@mu.org cell: 408-480-4684
On Sun, 12 Sep 2004, Giorgos Keramidas wrote: > He's probably the best person to suggest a fix for this warning, if one > is really needed. There is no need for fix a error as there are no error. The 'fd' is correctly initialized with no exception, althought GCC can't evaluate it. It's try to eliminate unnecesarry warning. > No. I don't know why you think that this is a good fix for all the > uninitialized pointer warnings. It's not. Never :-/ Why ? It's written within the "description" section of the PR. Warning shoult be "attention marks" - THIS CONSTRUCT SHOULD BE REVIEWED. If there are too many warnings related to correct code we can't use it for it purpose. Warnings become notice with almost no value. Well. I compiled the code and evaluate that hundreds of warnings didn't point to problematic code, but are "false" only. I will compile the code next month. Do you think I recognise there are hundreds + 1 warning, so we should evaulate this +1 warning to be sure there are no error ? Warnings may be very usefull information, unless they lie so much. Current code cause piles of false warnings. It "warnings" are hard to use as true warning ... But any commiter can close all of my PR if he think I'm not true ... > After a quick glance at the source of rpcbind() I think that there's no way > that my_xrpt can be used uninitialized. A proper fix for this warning > would be then to just set my_xrpt to NULL at first and let the rest of the > function unchanged regarding my_xrpt. The unnecesarry initialisation of variable initialised again later seems to be vaste of resources. But IMHO only.
On Sun, 12 Sep 2004, Alfred Perlstein wrote: > I'm having a hard time seeing these warnings, how are they > being triggered? -Wall -O3 The "O3" is not for production environment, of course. But evaluating of warnings can expose some of hidden bugs within code. The inspection become to be very hard work unless we eliminate pile of false warnings (despite the are not shown during "standard" compilation) ... Dan
On 2004-09-13 05:59, Dan Lukes <dan@obluda.cz> wrote: > Dima Dorfman wrote: > > Any initialization in the form "T v = v" invokes undefined behavior by > > using the indeterminate value of an object. Eliminating a warning or > > Unless compiler documentation say other ... > The v=v DURING DECLARATION (not later) is special case. > > Even on non GCC compiler it didn't make things worse - the value of v > has not been changed by v=v statement ... Undefined behavior is always worse. It means you cannot determine what the program will do until you run it, which is worse than setting a pointer to NULL, which might be equivalent to: char *p = 0; and translate to 1-2 machine instructions. > > compiler can't be convinced that the variable is never used before > > being initialized, please initialize it to something obviously bogus. > > It can be convicted, by "v=v" trick during declaration of variable. > It's hack, of course. A dangerous one too. See my post about a "trap" that might be generated when you try to set a pointer's value to something completely bogus. > It seems to be similar to "declared but newer used" warning. It is > eliminated by compiler specific hack also ("__unused")- not by true > usage of variable. That's a different thing. Not using a variable isn't really dangerous. Using the value of a pointer whose initial value contains garbage *is* dangerous. > If you thing those patches can't be used (with or without corrections > you recommended) then close the PR I think a lot of the work in these patches is useful. A few changes here, a minor fix there and it'll be nice if they're committed and have as many warnings fixed as possible. This is my personal opinion though. Keep up the good work ;-)
On 2004-09-13 02:57, Dan Lukes <dan@obluda.cz> wrote: >On Sun, 12 Sep 2004, Giorgos Keramidas wrote: >> After a quick glance at the source of rpcbind() I think that there's >> no way that my_xrpt can be used uninitialized. A proper fix for this >> warning would be then to just set my_xrpt to NULL at first and let >> the rest of the function unchanged regarding my_xrpt. > > The unnecesarry initialisation of variable initialised again later > seems to be vaste of resources. But IMHO only. True, but this is not the case here. If you do initialize the value of my_xrpt to something (i.e. to whatever value happens to be held in my_xrpt at the time execution reaches the point of its declaration), you should at least set it to something that is Right(TM). This initialization char *foo = foo; is not cheaper than this one: char *foo = NULL; so if you're going to pay the penalty of initializing a variable to shuttup the compiler, the second is preferrable because it explicitly sets the pointer to a value that cannot be dereferenced. This will catch possible bugs (current or future) in the rest of the code, so it's a lot safer.
Giorgos Keramidas wrote: > This initialization > char *foo = foo; > > is not cheaper than this one: > char *foo = NULL; Not true. The first one is optimized out by GCC even if O level is 0. Remember - it's hack/special construct (a special in-band way to tell the compiler something special) - it's not real assignment. Check it by your favorite disassembler. I did it. > This will catch possible bugs (current or future) in the rest of the code, so > it's a lot safer. I agree with it. Dan -- Dan Lukes, SISAL, MFF UK tel: +420 2 21914205, fax: +420 2 21914206 AKA: dan@obluda.cz, dan@freebsd.cz, dan@kolej.mff.cuni.cz, dan@fio.cz
Giorgos Keramidas wrote: > That's a different thing. Not using a variable isn't really dangerous. > Using the value of a pointer whose initial value contains garbage *is* > dangerous. Unless you accidentally use "len" instead of "slen" which is totally diferent variable. Even "declared but newer used" warning may warn agains dangerous error within the code. It's the reason why any warning should be carefully evaluated (it's the reason we should eliminate warnings to already evaluated code) > I think a lot of the work in these patches is useful. A few changes > here, a minor fix there and it'll be nice if they're committed and have > as many warnings fixed as possible. This is my personal opinion though. Well, I hope somebody commit it. > Keep up the good work ;-) Thanks. And thanks for suggestions also. Dan
On 2004-09-13 06:46, Dima Dorfman <dd@freebsd.org> wrote: > Dan Lukes <dan@obluda.cz> wrote: > > Dima Dorfman wrote: > > > Any initialization in the form "T v = v" invokes undefined behavior by > > > using the indeterminate value of an object. Eliminating a warning or > > > > Unless compiler documentation say other ... > > The v=v DURING DECLARATION (not later) is special case. > > I looked for such an exception in C99 before my original post, but I > didn't find one. There is a footnote in C99 that I'm a bit worried about. In page 37, a footnote reads: 41) Thus, an automatic variable can be initialized to a trap representation without causing undefined behavior, but the value of the variable cannot be used until a proper value is stored in it. Unless I'm mistaken, this means that the value of `foo' cannot be used in the right part of the following assignment: char *foo = foo; > Do I sound like a pedant yet? ;-) For a while, I was afraid my comments would sound unnecessarily pedantic too. Fortunately, it seems I'm not.
This patch isn't right. Taking and will develop a fix.
Keyword: patch or patch-ready – in lieu of summary line prefix: [patch] * bulk change for the keyword * summary lines may be edited manually (not in bulk). Keyword descriptions and search interface: <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>