gcc/toplev.c: Avoid to close 'asm_out_file' when it is 'stdout'

Submitted by Chen Gang on July 21, 2014, 3:47 p.m.

Details

Message ID 53CD3617.2000503@gmail.com
State New
Headers show

Commit Message

Chen Gang July 21, 2014, 3:47 p.m.
'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(-)

Comments

Jeff Law July 23, 2014, 3:44 a.m.
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
Chen Gang July 23, 2014, 10:17 p.m.
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.
Jeff Law July 25, 2014, 9:12 p.m.
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
Chen Gang July 26, 2014, 1:33 a.m.
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.

Patch hide | download patch | download mbox

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);
     }