| Message ID | 53CD3617.2000503@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 07/21/14 09:47, Chen Gang wrote: > 'asm_out_file' may be 'stdout', so need check this case before close it. > Or 'stdout' may be closed -- since need not open 'stdout', either need > not close it. > > ChangLog: > > * topleve.c (finalize): Avoid to close 'asm_out_file' when it is > 'stdout'. What exactly is the problem with closing stdout at this point? In general, you need to state the problem you're trying to fix with your patch. Jeff
On 07/23/2014 11:44 AM, Jeff Law wrote: > On 07/21/14 09:47, Chen Gang wrote: >> 'asm_out_file' may be 'stdout', so need check this case before close it. >> Or 'stdout' may be closed -- since need not open 'stdout', either need >> not close it. >> >> ChangLog: >> >> * topleve.c (finalize): Avoid to close 'asm_out_file' when it is >> 'stdout'. > What exactly is the problem with closing stdout at this point? In general, you need to state the problem you're trying to fix with your patch. > Excuse me, I only find it by reading source code, so for me, I didn't meet the real problem for it, so at least, this patch is not urgent ( although I am not sure whether it is still valuable or not). At present, I am a newbie, and use 2 ways to learn gcc and binutils. - Cross compile the cross compiler with '-W' for linux kernel. (If find issues, I shall try to fix them with related members). - Reading source code of gcc and binutils, if find some where can be improved, and try to send patch for it. By the way, is there a trivial patch mailing list of gcc? I guess most of my patches belong to trivial. Thanks.
On 07/23/14 16:17, Chen Gang wrote: > On 07/23/2014 11:44 AM, Jeff Law wrote: >> On 07/21/14 09:47, Chen Gang wrote: >>> 'asm_out_file' may be 'stdout', so need check this case before close it. >>> Or 'stdout' may be closed -- since need not open 'stdout', either need >>> not close it. >>> >>> ChangLog: >>> >>> * topleve.c (finalize): Avoid to close 'asm_out_file' when it is >>> 'stdout'. >> What exactly is the problem with closing stdout at this point? In general, you need to state the problem you're trying to fix with your patch. >> > > Excuse me, I only find it by reading source code, so for me, I didn't > meet the real problem for it, so at least, this patch is not urgent ( > although I am not sure whether it is still valuable or not). OK. I suspected it might be the case that you just saw something odd and sent a patch to change it. But that was just a suspicion -- there well could have been some path you found where GCC wanted to write to stdout after the point where we closed the output file. That would clearly be a bug and warrant fixing immediately. Either way it's important you tell us why you're making a change in a way which allows us to evaluate the importance of the change. Otherwise we have to guess and we could easily guess wrong. In this specific instance, I don't really see any value in avoiding the close of stdout. > > At present, I am a newbie, and use 2 ways to learn gcc and binutils. > > - Cross compile the cross compiler with '-W' for linux kernel. > (If find issues, I shall try to fix them with related members). > > - Reading source code of gcc and binutils, if find some where can be > improved, and try to send patch for it. And these are excellent ways to get started. Another would be to build GCC with Clang/LLVM and see what warnings that generates and try to fix them. Perusing the bug database (gcc.gnu.org/bugzilla) can sometimes result in identifying issues you can resolve. > > By the way, is there a trivial patch mailing list of gcc? I guess most > of my patches belong to trivial. No, all patches go to gcc-patches, even trivial ones. Thanks, jeff
On 07/26/2014 05:12 AM, Jeff Law wrote: > On 07/23/14 16:17, Chen Gang wrote: >> On 07/23/2014 11:44 AM, Jeff Law wrote: >>> On 07/21/14 09:47, Chen Gang wrote: >>>> 'asm_out_file' may be 'stdout', so need check this case before close >>>> it. >>>> Or 'stdout' may be closed -- since need not open 'stdout', either need >>>> not close it. >>>> >>>> ChangLog: >>>> >>>> * topleve.c (finalize): Avoid to close 'asm_out_file' when it is >>>> 'stdout'. >>> What exactly is the problem with closing stdout at this point? In >>> general, you need to state the problem you're trying to fix with your >>> patch. >>> >> >> Excuse me, I only find it by reading source code, so for me, I didn't >> meet the real problem for it, so at least, this patch is not urgent ( >> although I am not sure whether it is still valuable or not). > OK. I suspected it might be the case that you just saw something odd > and sent a patch to change it. > > But that was just a suspicion -- there well could have been some path > you found where GCC wanted to write to stdout after the point where we > closed the output file. That would clearly be a bug and warrant fixing > immediately. > In source root directory: "grep -rn asm_out_file * | grep stdout" "grep -rn asm_out_file * | grep fclose" "grep -rn asm_out_file * | grep NULL". We can know about stdout may be used (e.g. the related parameter is '-') in init_asm_output() in "gcc/toplev.c". And only one place to close it in finalize() in "gcc/toplev.c". But really, it is only by reading source code, no any real test for it, so yeah, we can say it is only a suspicion. > Either way it's important you tell us why you're making a change in a > way which allows us to evaluate the importance of the change. Otherwise > we have to guess and we could easily guess wrong. > > In this specific instance, I don't really see any value in avoiding the > close of stdout. > I don't see either, so at least, it is not urgent. But if it was really a bug, for me, we have to fix it, when we have time. >> >> At present, I am a newbie, and use 2 ways to learn gcc and binutils. >> >> - Cross compile the cross compiler with '-W' for linux kernel. >> (If find issues, I shall try to fix them with related members). >> >> - Reading source code of gcc and binutils, if find some where can be >> improved, and try to send patch for it. > And these are excellent ways to get started. Another would be to build > GCC with Clang/LLVM and see what warnings that generates and try to fix > them. Perusing the bug database (gcc.gnu.org/bugzilla) can sometimes > result in identifying issues you can resolve. > Thank you for your ideas and suggestions, Clang/LLVM and bugzilla are really two important and valuable places. But excuse me, I have no enough time resources on it. I have started Linux kernel and qemu/kvm/xen in my free time, and now, I am starting gcc/binutils in my free time, too. - "cross compiling the cross-compiler for linux kernel, and let each architectures in kernel pass allmodconfig" is an efficient way to me for both learning gcc/binutils and linux kernel. - Reading source code are always necessary in any cases (it is my main learning way for qemu/kvm/xen). Clang/LLVM and bugzilla are really very important, so I shall try them in the future (I guess, may be next year -- 2015). >> By the way, is there a trivial patch mailing list of gcc? I guess most >> of my patches belong to trivial. > No, all patches go to gcc-patches, even trivial ones. > OK, thanks. And I shall continue in gcc-patches. And excuse me, next, most of my trivial patches maybe bother many other members in gcc-patches mailing list. Thanks.
diff --git a/gcc/toplev.c b/gcc/toplev.c index 1c9befd..5fc11ae 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1878,7 +1878,7 @@ finalize (bool no_backend) { if (ferror (asm_out_file) != 0) fatal_error ("error writing to %s: %m", asm_file_name); - if (fclose (asm_out_file) != 0) + if (asm_out_file != stdout && fclose (asm_out_file) != 0) fatal_error ("error closing %s: %m", asm_file_name); }
'asm_out_file' may be 'stdout', so need check this case before close it. Or 'stdout' may be closed -- since need not open 'stdout', either need not close it. ChangLog: * topleve.c (finalize): Avoid to close 'asm_out_file' when it is 'stdout'. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- gcc/toplev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)