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

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

[Chen Gang]

'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(-)

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

Re: [PATCH] gcc/toplev.c: Avoid to close 'asm_out_file' when it is 'stdout'

[Jeff Law]

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

Re: [PATCH] gcc/toplev.c: Avoid to close 'asm_out_file' when it is 'stdout'

[Chen Gang]

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.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [PATCH] gcc/toplev.c: Avoid to close 'asm_out_file' when it is 'stdout'

[Jeff Law]

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

Re: [PATCH] gcc/toplev.c: Avoid to close 'asm_out_file' when it is 'stdout'

[Chen Gang]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed