| Message ID | 1456160612-25123-1-git-send-email-aconole@bytheb.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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 <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
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
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); }