[PATCH] gcov: Configurable destination for error output

[PATCH] gcov: Configurable destination for error output

[Aaron Conole]

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
silent for certain tests. This change allows configuring the gcov output by
an environment variable which will be used to open the appropriate file.
---
v0: Note, I do not have a signed FSF agreement at present. I do work at Red
Hat, but gcc is not my primary role. If this change requires paperwork, I can
sign and return asap.

 libgcc/libgcov-driver-system.c | 28 +++++++++++++++++++++++++++-
 libgcc/libgcov-driver.c        |  5 +++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/libgcc/libgcov-driver-system.c b/libgcc/libgcov-driver-system.c
index 4e3b244..09bb167 100644
--- a/libgcc/libgcov-driver-system.c
+++ b/libgcc/libgcov-driver-system.c
@@ -23,6 +23,8 @@ 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/>.  */
 
+static FILE *gcov_error_file;
+
 /* A utility function for outputing errors.  */
 
 static int __attribute__((format(printf, 1, 2)))
@@ -31,11 +33,35 @@ gcov_error (const char *fmt, ...)
   int ret;
   va_list argp;
   va_start (argp, fmt);
-  ret = vfprintf (stderr, fmt, argp);
+  ret = vfprintf (gcov_error_file, fmt, argp);
   va_end (argp);
   return ret;
 }
 
+static void
+gcov_error_exit(void)
+{
+  if (gcov_error_file != stderr)
+	{
+	  fclose(gcov_error_file);
+	}
+}
+
+static void
+gcov_error_init(void)
+{
+  char *gcov_error_filename = getenv("GCOV_ERROR_FILE");
+  gcov_error_file = NULL;
+  if (gcov_error_filename)
+	{
+	  FILE *openfile = fopen(gcov_error_filename, "w");
+	  if (openfile)
+		gcov_error_file = openfile;
+	}
+  if (!gcov_error_file)
+	gcov_error_file = stderr;
+}
+
 /* Make sure path component of the given FILENAME exists, create
    missing directories. FILENAME must be writable.
    Returns zero on success, or -1 if an error occurred.  */
diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
index 9c4eeca..082755a 100644
--- a/libgcc/libgcov-driver.c
+++ b/libgcc/libgcov-driver.c
@@ -45,6 +45,8 @@ void __gcov_init (struct gcov_info *p __attribute__ ((unused))) {}
 
 /* A utility function for outputing errors.  */
 static int gcov_error (const char *, ...);
+static void gcov_error_init(void);
+static void gcov_error_exit(void);
 
 #include "gcov-io.c"
 
@@ -878,6 +880,8 @@ gcov_exit (void)
     __gcov_root.prev->next = __gcov_root.next;
   else
     __gcov_master.root = __gcov_root.next;
+
+  gcov_error_exit();
 }
 
 /* Add a new object file onto the bb chain.  Invoked automatically
@@ -900,6 +904,7 @@ __gcov_init (struct gcov_info *info)
 		__gcov_master.root->prev = &__gcov_root;
 	      __gcov_master.root = &__gcov_root;
 	    }
+	  gcov_error_init();
 	  atexit (gcov_exit);
 	}
 
-- 
2.5.0

Re: [PATCH] gcov: Configurable destination for error output

[Nathan Sidwell]

On 02/22/16 12:03, 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
> silent for certain tests. This change allows configuring the gcov output by
> an environment variable which will be used to open the appropriate file.

Why is the invoker unable to direct fd2 before execing gcov?

nathan

Re: [PATCH] gcov: Configurable destination for error output

[Aaron Conole]

Nathan Sidwell <nathan@acm.org> writes:

Hi Nathan, thanks so much for looking at this!

> On 02/22/16 12:03, 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
>> silent for certain tests. This change allows configuring the gcov output by
>> an environment variable which will be used to open the appropriate file.
>
> Why is the invoker unable to direct fd2 before execing gcov?

I want to make sure I understand your question. You are asking why
something like: ` ./program_executed 2>/path/to/file `` is not preferred?

If this is the question, the problem is program errors will be intermingled
with gcov error messages. Let's suppose that I've got a unit test
program (since that's what I have as specifically happening). I expect
certain tests from that program to spit out errors on stderr. So, I
filter out that text in stderr, so normal case stderr results will be
clear. Now, I build with gcov enabled. In this case, gcov writes
'profiling:...' to stderr. Now, the test fails, even though nothing
changed apart from using gcov.

Sometimes this is a real user error (clean .gcda and .gcno files, do
clean build and then rerun), but other times, for instance if I use
openvswitch's 'make check', there is no rebuild which happens, but gcov
has merge mismatch errors anyway. I want to not squelch those errors,
but I don't want them intermingled with the program's error output.

I hope I explained myself and answered your question. Please let me know
if I didn't.

Thanks again,
-Aaron

> nathan

Re: [PATCH] gcov: Configurable destination for error output

[Nathan Sidwell]

On 02/22/16 13:11, Aaron Conole wrote:
> Nathan Sidwell <nathan@acm.org> writes:
>
> Hi Nathan, thanks so much for looking at this!
>
>> On 02/22/16 12:03, 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
>>> silent for certain tests. This change allows configuring the gcov output by
>>> an environment variable which will be used to open the appropriate file.
>>
>> Why is the invoker unable to direct fd2 before execing gcov?
>
> I want to make sure I understand your question. You are asking why
> something like: ` ./program_executed 2>/path/to/file `` is not preferred?
>
> If this is the question, the problem is program errors will be intermingled
> with gcov error messages. Let's suppose that I've got a unit test
> program (since that's what I have as specifically happening). I expect
> certain tests from that program to spit out errors on stderr. So, I
> filter out that text in stderr, so normal case stderr results will be
> clear. Now, I build with gcov enabled. In this case, gcov writes
> 'profiling:...' to stderr. Now, the test fails, even though nothing
> changed apart from using gcov.

ah, gotcha!  I'd not understood this was in the gcov runtime (as opposed to the 
gcov analysis programs).

Doesn't your use of statics result in multiple fopens in different shared 
objects, and subsequent tangling buffering?  It would seem to me that 
gcov_error_file needs the same linkage as __gcov_master?  Would lazy opening and 
"w+" be better behaviour?

BTW, the code formatting needs correcting.

nathan

Re: [PATCH] gcov: Configurable destination for error output

[Aaron Conole]

Nathan Sidwell <nathan@acm.org> writes:

> On 02/22/16 13:11, Aaron Conole wrote:
>> Nathan Sidwell <nathan@acm.org> writes:
>>
>> Hi Nathan, thanks so much for looking at this!
>>
>>> On 02/22/16 12:03, 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
>>>> silent for certain tests. This change allows configuring the gcov output by
>>>> an environment variable which will be used to open the appropriate file.
>>>
>>> Why is the invoker unable to direct fd2 before execing gcov?
>>
>> I want to make sure I understand your question. You are asking why
>> something like: ` ./program_executed 2>/path/to/file `` is not preferred?
>>
>> If this is the question, the problem is program errors will be intermingled
>> with gcov error messages. Let's suppose that I've got a unit test
>> program (since that's what I have as specifically happening). I expect
>> certain tests from that program to spit out errors on stderr. So, I
>> filter out that text in stderr, so normal case stderr results will be
>> clear. Now, I build with gcov enabled. In this case, gcov writes
>> 'profiling:...' to stderr. Now, the test fails, even though nothing
>> changed apart from using gcov.
>
> ah, gotcha!  I'd not understood this was in the gcov runtime (as
> opposed to the gcov analysis programs).

I'll update $subject to try and make it clearer. Thanks!

> Doesn't your use of statics result in multiple fopens in different
> shared objects, and subsequent tangling buffering? It would seem to
> me that gcov_error_file needs the same linkage as __gcov_master?
> Would lazy opening and "w+" be better behaviour?

D'oh, you're probably right. In my excitement to contribute, I forgot
this was shared. I think 'w' should be correct, since this isn't
intended to be read at all, but I could be convinced otherwise.

By lazy load, do you mean only after the first gcov_error call? That's
probably a better approach, and I can recook, test, and resubmit with
these corrected.

> BTW, the code formatting needs correcting.

Okay. Emacs is autoformatting it to gnu style for me (I thought), is
there one I should prefer for gcc contributions?

> nathan

Re: [PATCH] gcov: Configurable destination for error output

[Aaron Conole]

Aaron Conole <aconole@bytheb.org> writes:

> Nathan Sidwell <nathan@acm.org> writes:
>
>> On 02/22/16 13:11, Aaron Conole wrote:
>>> Nathan Sidwell <nathan@acm.org> writes:
>>>
>>> Hi Nathan, thanks so much for looking at this!
>>>
>>>> On 02/22/16 12:03, 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
>>>>> silent for certain tests. This change allows configuring the gcov output by
>>>>> an environment variable which will be used to open the appropriate file.
>>>>
>>>> Why is the invoker unable to direct fd2 before execing gcov?
>>>
>>> I want to make sure I understand your question. You are asking why
>>> something like: ` ./program_executed 2>/path/to/file `` is not preferred?
>>>
>>> If this is the question, the problem is program errors will be intermingled
>>> with gcov error messages. Let's suppose that I've got a unit test
>>> program (since that's what I have as specifically happening). I expect
>>> certain tests from that program to spit out errors on stderr. So, I
>>> filter out that text in stderr, so normal case stderr results will be
>>> clear. Now, I build with gcov enabled. In this case, gcov writes
>>> 'profiling:...' to stderr. Now, the test fails, even though nothing
>>> changed apart from using gcov.
>>
>> ah, gotcha!  I'd not understood this was in the gcov runtime (as
>> opposed to the gcov analysis programs).
>
> I'll update $subject to try and make it clearer. Thanks!
>
>> Doesn't your use of statics result in multiple fopens in different
>> shared objects, and subsequent tangling buffering? It would seem to
>> me that gcov_error_file needs the same linkage as __gcov_master?
>> Would lazy opening and "w+" be better behaviour?
>
> D'oh, you're probably right. In my excitement to contribute, I forgot
> this was shared. I think 'w' should be correct, since this isn't
> intended to be read at all, but I could be convinced otherwise.
>
> By lazy load, do you mean only after the first gcov_error call? That's
> probably a better approach, and I can recook, test, and resubmit with
> these corrected.
>
>> BTW, the code formatting needs correcting.
>
> Okay. Emacs is autoformatting it to gnu style for me (I thought), is
> there one I should prefer for gcc contributions?

Ignore this part, just realized what was going on and it will be correct
in the next version.

>> nathan

Re: [PATCH] gcov: Configurable destination for error output

[Nathan Sidwell]

On 02/22/16 14:35, Aaron Conole wrote:

> D'oh, you're probably right. In my excitement to contribute, I forgot
> this was shared. I think 'w' should be correct, since this isn't
> intended to be read at all, but I could be convinced otherwise.

sorry, I misremembered the encoding of write append, which is "a" -- don't 
clobber the existing contents.  I think that's the usual behaviour for error 
logging (.xsession-error and the like).

> By lazy load, do you mean only after the first gcov_error call? That's
> probably a better approach, and I can recook, test, and resubmit with
> these corrected.

Lazy opening -- open on first error output.   Something like

if (!gcov_error_file)
   gcov_error_file = gcov_open_error_file ();

in gcov_error?

FWIW, I think this has missed the boat for gcc 6.1, as we're now in stage  4.

nathan

-- 
Nathan Sidwell

Re: [PATCH] gcov: Configurable destination for error output

[Aaron Conole]

Nathan Sidwell <nathan@codesourcery.com> writes:

> On 02/22/16 14:35, Aaron Conole wrote:
>
>> D'oh, you're probably right. In my excitement to contribute, I forgot
>> this was shared. I think 'w' should be correct, since this isn't
>> intended to be read at all, but I could be convinced otherwise.
>
> sorry, I misremembered the encoding of write append, which is "a" --
> don't clobber the existing contents.  I think that's the usual
> behaviour for error logging (.xsession-error and the like).

Done.

>> By lazy load, do you mean only after the first gcov_error call? That's
>> probably a better approach, and I can recook, test, and resubmit with
>> these corrected.
>
> Lazy opening -- open on first error output.   Something like
>
> if (!gcov_error_file)
>   gcov_error_file = gcov_open_error_file ();
>
> in gcov_error?

Before I start cooking up this change, is it possible I need to worry about
gcov_error being invoked from multiple threads? If so, I'll need some
kind of mutex which I think is not needed with the current design.

> FWIW, I think this has missed the boat for gcc 6.1, as we're now in stage  4.

No worries.

-Aaron

> nathan

Re: [PATCH] gcov: Configurable destination for error output

[Nathan Sidwell]

On 02/23/16 11:04, Aaron Conole wrote:

> Before I start cooking up this change, is it possible I need to worry about
> gcov_error being invoked from multiple threads? If so, I'll need some
> kind of mutex which I think is not needed with the current design.

As I recall the main entry points to the gcov machinery (__gcov_dump etc) have a 
lock already.  So I think you're ok.

nathan