Re: [PATCH v2] gcov: Runtime configurable destination output

[Aaron Conole <aconole@redhat.com>]

Nathan Sidwell <nathan@acm.org> writes:

> On 02/24/16 16:52, Aaron Conole wrote:
>> The previous gcov behavior was to always output errors on the stderr channel.
>> This is fine for most uses, but some programs will require stderr to be
>> untouched by libgcov for certain tests. This change allows configuring
>> the gcov output via an environment variable which will be used to open
>> the appropriate file.
>
> this is ok in principle.  I have a couple of questions & nits below though.

Thank you for the consideration.  I will be submitting a new patch that
I hope fully addresses your comments below, either tomorrow or Monday.

Thanks so much for the review.

> I don't see a previous commit from you -- do you have a copyright
> assignment with the FSF? (although this patch is simple, my guess is
> the idea it implements is sufficiently novel to need one).  We can
> handle that off list.

I'm happy to report that I did send in some FSF paperwork this week.
Hopefully it is on record now, but even if it isn't I live a train ride
away from the FSF headquarters so I'd be happy to take the time to make
sure it's all signed correctly.

>> diff --git a/libgcc/libgcov-driver-system.c b/libgcc/libgcov-driver-system.c
>> index 4e3b244..0eb9755 100644
>> --- a/libgcc/libgcov-driver-system.c
>> +++ b/libgcc/libgcov-driver-system.c
>> @@ -23,6 +23,24 @@ a copy of the GCC Runtime Library Exception along
>> with this program;
>>  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>>  <http://www.gnu.org/licenses/>.  */
>>
>> +FILE *__gcov_error_file = NULL;
>
> Unless I'm missing something, isn't this only accessed from this file?
> (So could be static with a non-underbarred name)

Ack.

>> @@ -30,12 +48,27 @@ gcov_error (const char *fmt, ...)
>>  {
>>    int ret;
>>    va_list argp;
>> +
>> +  if (!__gcov_error_file)
>> +    __gcov_error_file = get_gcov_error_file();
>
> Needs space before ()

Ack.

>> +
>>    va_start (argp, fmt);
>> -  ret = vfprintf (stderr, fmt, argp);
>> +  ret = vfprintf (__gcov_error_file, fmt, argp);
>>    va_end (argp);
>>    return ret;
>>  }
>>
>> +#if !IN_GCOV_TOOL
>
> And this protection here, makes me wonder what happens if one is
> IN_GCOV_TOOL. Does it pay attention to GCOV_ERROR_FILE?  That would
> seem incorrect, and thus the above should be changed so that stderr is
> unconditionally used when IN_GCOV_TOOL?

You are correct.  I will fix it.

>> +static void
>> +gcov_error_exit(void)
>> +{
>> +  if (__gcov_error_file && __gcov_error_file != stderr)
>> +    {
>
> Braces are not needed here.

Ack.

>> --- a/libgcc/libgcov-driver.c
>> +++ b/libgcc/libgcov-driver.c
>> @@ -46,6 +46,10 @@ void __gcov_init (struct gcov_info *p
>> __attribute__ ((unused))) {}
>
>> +  gcov_error_exit();
>
> Needs space before ().

Ack.

> nathan