# possible code error at run_command.c 11 messages from 2009-12-21 to 2009-12-21. Participants: Frank Li, Johannes Sixt, Wincent Colaiuta, Erik Faye-Lund. Thread: https://gitlist.dev/t/21998 ## Frank Li, 2009-12-21 06:46 Subject: possible code error at run_command.c Message-ID: <1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com> URL: https://gitlist.dev/e/1976ea660912202246k45732bf2p111bbeb78047693e%40mail.gmail.com ``` int start_command(struct child_process *cmd) { int need_in, need_out, need_err; int fdin[2], fdout[2], fderr[2]; int failed_errno = failed_errno; I have not found failed_errno as global variable. failed_errno = failed_errno means nothing. It is possible coding error and should be int failed_errno= 0 or failed_errno=errno. ``` ## Wincent Colaiuta, 2009-12-21 07:09 Subject: Re: possible code error at run_command.c Message-ID: <2E55A685-834C-4C64-99F1-4060FEC7C2DB@wincent.com> URL: https://gitlist.dev/e/2E55A685-834C-4C64-99F1-4060FEC7C2DB%40wincent.com In-Reply-To: <1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com> ``` El 21/12/2009, a las 07:46, Frank Li escribió: > int start_command(struct child_process *cmd) > { > int need_in, need_out, need_err; > int fdin[2], fdout[2], fderr[2]; > int failed_errno = failed_errno; > > I have not found failed_errno as global variable. > failed_errno = failed_errno means nothing. > > It is possible coding error and should be > int failed_errno= 0 or > failed_errno=errno. Via "git blame": commit 5a7a3671b74c043216549b94a718da04cc3ffcd6 Author: David Soria Parra Date: Tue Aug 4 11:28:40 2009 +0200 run-command.c: squelch a "use before assignment" warning i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490) compiler (and probably others) mistakenly thinks variable failed_errno is used before assigned. Work it around by giving it a fake initialization. Signed-off-by: David Soria Parra Signed-off-by: Junio C Hamano Cheers, Wincent ``` ## Johannes Sixt, 2009-12-21 07:18 Subject: Re: possible code error at run_command.c Message-ID: <4B2F214D.2020905@viscovery.net> URL: https://gitlist.dev/e/4B2F214D.2020905%40viscovery.net In-Reply-To: <1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com> ``` Frank Li schrieb: > int start_command(struct child_process *cmd) > { > int need_in, need_out, need_err; > int fdin[2], fdout[2], fderr[2]; > int failed_errno = failed_errno; > > I have not found failed_errno as global variable. > failed_errno = failed_errno means nothing. This is a commonly used idiom to avoid an (incorrect) compiler warning about an uninitialized variable. Strictly speaking, I think that you are right by saying "means nothing" because the use of the uninitialized variable invokes undefined behavior (and for this reason, I dislike this construct), but in practice it will not make a difference. -- Hannes ``` ## Frank Li, 2009-12-21 07:29 Subject: Re: possible code error at run_command.c Message-ID: <1976ea660912202329x42f6add3j9175867e8723a4bd@mail.gmail.com> URL: https://gitlist.dev/e/1976ea660912202329x42f6add3j9175867e8723a4bd%40mail.gmail.com In-Reply-To: <4B2F214D.2020905@viscovery.net> ``` > This is a commonly used idiom to avoid an (incorrect) compiler warning > about an uninitialized variable. > > Strictly speaking, I think that you are right by saying "means nothing" > because the use of the uninitialized variable invokes undefined behavior > (and for this reason, I dislike this construct), but in practice it will > not make a difference. > This error is captured at MSVC environment by run time varible check. I prefer change it to int failed_errno = errno; ``` ## Johannes Sixt, 2009-12-21 07:43 Subject: Re: possible code error at run_command.c Message-ID: <4B2F2727.4060405@viscovery.net> URL: https://gitlist.dev/e/4B2F2727.4060405%40viscovery.net In-Reply-To: <1976ea660912202329x42f6add3j9175867e8723a4bd@mail.gmail.com> ``` Frank Li schrieb: >> This is a commonly used idiom to avoid an (incorrect) compiler warning >> about an uninitialized variable. >> >> Strictly speaking, I think that you are right by saying "means nothing" >> because the use of the uninitialized variable invokes undefined behavior >> (and for this reason, I dislike this construct), but in practice it will >> not make a difference. >> > > This error is captured at MSVC environment by run time varible check. Disable this check - it just takes away performance. :-) (If you don't disable the check, then keep the required changes private.) > I prefer change it to > int failed_errno = errno; You don't need to initialize the variable at all because it is always initialized elsewhere before it is used. Perhaps MSVC is clever enough to see it? -- Hannes ``` ## Frank Li, 2009-12-21 08:18 Subject: Re: possible code error at run_command.c Message-ID: <1976ea660912210018y15acfe32o78841d5e0968f793@mail.gmail.com> URL: https://gitlist.dev/e/1976ea660912210018y15acfe32o78841d5e0968f793%40mail.gmail.com In-Reply-To: <4B2F2727.4060405@viscovery.net> ``` > > Disable this check - it just takes away performance. :-) Release version will disable this. Debug version enable this by default is just for find out code error. > > (If you don't disable the check, then keep the required changes private.) > >> I prefer change it to >> int failed_errno = errno; > > You don't need to initialize the variable at all because it is always > initialized elsewhere before it is used. Perhaps MSVC is clever enough to > see it? > Maybe some excute path miss initialized it. Otherwise compiler will not report warning. ``` ## Johannes Sixt, 2009-12-21 08:30 Subject: Re: possible code error at run_command.c Message-ID: <4B2F320A.6010908@viscovery.net> URL: https://gitlist.dev/e/4B2F320A.6010908%40viscovery.net In-Reply-To: <1976ea660912210018y15acfe32o78841d5e0968f793@mail.gmail.com> ``` Frank Li schrieb: > Maybe some excute path miss initialized it. Otherwise compiler will > not report warning. LOOK AT THE CODE. There is no such code path. -- Hannes ``` ## Erik Faye-Lund, 2009-12-21 08:41 Subject: Re: possible code error at run_command.c Message-ID: <40aa078e0912210041g1fd94c77g6cf9f1b236b6ecd7@mail.gmail.com> URL: https://gitlist.dev/e/40aa078e0912210041g1fd94c77g6cf9f1b236b6ecd7%40mail.gmail.com In-Reply-To: <4B2F320A.6010908@viscovery.net> ``` On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt wrote: > Frank Li schrieb: >> Maybe some excute path miss initialized it. Otherwise compiler will >> not report warning. > > LOOK AT THE CODE. There is no such code path. > That's odd. I agree, there isn't such a code path. But this is the first time I've ever seen this MSVC-feature turn up false positives, which puzzles me. Perhaps Frank has had to modify start_command for MSVC, and somehow introduced such an error? I dunno. -- Erik "kusma" Faye-Lund ``` ## Johannes Sixt, 2009-12-21 08:52 Subject: Re: possible code error at run_command.c Message-ID: <4B2F3732.6030903@viscovery.net> URL: https://gitlist.dev/e/4B2F3732.6030903%40viscovery.net In-Reply-To: <40aa078e0912210041g1fd94c77g6cf9f1b236b6ecd7@mail.gmail.com> ``` Erik Faye-Lund schrieb: > On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt wrote: >> Frank Li schrieb: >>> Maybe some excute path miss initialized it. Otherwise compiler will >>> not report warning. >> LOOK AT THE CODE. There is no such code path. >> > > That's odd. Only if Frank did the homework and removed the initialization from int failed_errno = failed_errno; > I agree, there isn't such a code path. But this is the > first time I've ever seen this MSVC-feature turn up false positives, > which puzzles me. This line will trigger the check. It initializes failed_errno with itself, which is still uninitialized at this time. Note that we have more definitions of this kind: $ git grep -E ' ([a-zA-Z_][a-zA-Z_0-9]*) = \1[;,]' *.c builtin-rev-list.c: int reaches = reaches, all = all; match-trees.c: unsigned mode1 = mode1; match-trees.c: unsigned mode2 = mode2; run-command.c: int failed_errno = failed_errno; transport.c: int cmp = cmp, len; wt-status.c: int status = status; -- Hannes ``` ## Erik Faye-Lund, 2009-12-21 09:08 Subject: Re: possible code error at run_command.c Message-ID: <40aa078e0912210108y4ea67dbfs2025bae20bdfb381@mail.gmail.com> URL: https://gitlist.dev/e/40aa078e0912210108y4ea67dbfs2025bae20bdfb381%40mail.gmail.com In-Reply-To: <4B2F3732.6030903@viscovery.net> ``` On Mon, Dec 21, 2009 at 9:52 AM, Johannes Sixt wrote: > Erik Faye-Lund schrieb: >> On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt wrote: >>> Frank Li schrieb: >>>> Maybe some excute path miss initialized it. Otherwise compiler will >>>> not report warning. >>> LOOK AT THE CODE. There is no such code path. >>> >> >> That's odd. > > Only if Frank did the homework and removed the initialization from > >        int failed_errno = failed_errno; > >> I agree, there isn't such a code path. But this is the >> first time I've ever seen this MSVC-feature turn up false positives, >> which puzzles me. > > This line will trigger the check. It initializes failed_errno with itself, > which is still uninitialized at this time. > > Note that we have more definitions of this kind: > > $ git grep -E ' ([a-zA-Z_][a-zA-Z_0-9]*) = \1[;,]' *.c > builtin-rev-list.c:             int reaches = reaches, all = all; > match-trees.c:          unsigned mode1 = mode1; > match-trees.c:          unsigned mode2 = mode2; > run-command.c:  int failed_errno = failed_errno; > transport.c:            int cmp = cmp, len; > wt-status.c:    int status = status; > > -- Hannes > > Right, I didn't think of that. Since that is the case, I'd say disabling run-time checks is perfectly sane. That being said, it might make sense for MSVC people to be able to have this feature turned on so they can easier find REAL programmer-errors. But I guess removing the self-assignments makes trouble for non-MSVC people. Perhaps some of these warnings could be avoided in a "safer" way? IMO, assigning variables to themselves like this is a bit fishy, as it effectively hides uninitialized-use warnings for the entire lifetime of that variable. Sure, the programmer who did that probably knows what he or she does -- but they can't possibly know what people will do in the future. If we could somehow turn these (really few) occasions into something that produces reliable warnings on uninitialized variables, I think we might be able to catch some bugs earlier. -- Erik "kusma" Faye-Lund ``` ## Frank Li, 2009-12-21 09:13 Subject: Re: possible code error at run_command.c Message-ID: <1976ea660912210113k6a8b05abwf86a0ea4f74aa997@mail.gmail.com> URL: https://gitlist.dev/e/1976ea660912210113k6a8b05abwf86a0ea4f74aa997%40mail.gmail.com In-Reply-To: <4B2F3732.6030903@viscovery.net> ``` > This line will trigger the check. It initializes failed_errno with itself, > which is still uninitialized at this time. > I see. I always use release version at finial production. This is not important. I read code again. There are not path. Thank you take care. ```