exit behavior of forked processed messes with lcov/gcov about cgreen HOT 28 CLOSED

ok - I figured out the problem.

Part of change 410 (by Thomas) changed some exit behaviour as follows:

 static void stop() {
-    exit(EXIT_SUCCESS);
+    _exit(EXIT_SUCCESS);
 }

Before this change my coverage stuff works as expected. After it doesn't.

So I suspect gcov is not functional with cgreen (and child processes)
for everyone in cgreen trunk (not just me).

I found a reference to this elsewhere:
http://mysql-qa.blogspot.co.uk/2009/06/kill-exit-exit-and-issues-getting-gcov.html

Any suggestions for a fix? Clearly I want gcov to work but I'm sure
Thomas wants his buffered output fix just as much.

from cgreen.

Just reproduced on:

• Ubuntu 12.04 (gcc 4.6.3 , lcov 1.9-2)
• Debian 8 (gcc 4.9.2, lcov 1.11)

the main thing we need to do is work out whether there are any negative consequences to my proposed patch to the _exit behavior

@thoni56 IIRC my concern about the fix outlined in my last comment was that despite it fixing the coverage reporting on the forked processes it would would lose the original benefit of your buffered output change:
http://sourceforge.net/p/cgreen/code/410/

Any ideas how we can retain both?

from cgreen.

A possible fix is to run the tests without fork()ing (see issue #5). For producing coverage information one would have to run the tests in-process.

A different solution would be to manually call __gcov_flush() before _exit(). This would have to be enabled or disabled at configure time with cmake.

from cgreen.

One suggestion would be to do what Jeb did in the post referred. Wrap it in #ifdef GCOV. That would mean that you get a choice, ungarbled output or coverage. I'm thinking that if your gcov'ing you would not care that much about posssibly garbled output or even run in an environment where you wouldn't even notice.

from cgreen.

@d-meiser I'm not sure if disabling forks would be a robust solution as it seems like we would lose the sandboxing we get by each test having its own child process. i.e. I'm wondering if static variables would get confused between tests etc.

calling __gcov_flush() before _exit() sounds pretty good as long as we are happy that cgreen itself is required to link against gcov to provide that symbol

@thoni56 I wasn't sure about that solution in the sense that since cgreen is a library that may have one definitive version installed on your system and so the idea of having to have 2 distinct versions (a "coverage friendly but non-ideal output" version and a "non coverage friendly but ideal output" version) of the library seems like it creates potentially klunkly problems for packaging/deployment. of course if every project has their own build of cgreen then I guess that is less of a concern.

another option would be an environment variable:

CGREEN_EXIT_WITH_FLUSH=1 ./runner

and then have the library decide which exit to call at runtime (which would mean no requireiement to have separate builds of the library to control this)

just some thoughts

from cgreen.

@gardenia good points. I think it was a mistake by me to mash together _exit() behaviour with the gcov stuff in PR #7 . I'll modify my PR to make these things more orthogonal. I propose adding __gcov_flush() before _exit()/exit(), protected by an #ifdef WITH_COVERAGE. The mechanism by which the child processes are terminated is then disentangled from the gcov issue.

from cgreen.

@gardenia I like the environment variable idea!

I'm in a situation where I'm already juggling with various versions of cgreen libraries (cygwin seems to require different libs for C/C++ and then there's the 32/64-bit issue on top of that...), so I guess I didn't think about how beautiful the world could be ;-)

from cgreen.

So, after #7, where are we with this? A user can still not run with coverage?

from cgreen.

Correct, #7 is orthogonal to this issue. I had the impression we were converging on addressing this by #5 where the in-process vs. fork() running of tests would be controlled by an environment variable. In the documentation we would then have to explain that coverage information can only be obtained with tests run in-process.

I think @matthargett had some code that addresses #5 so I left my fingers off of this.

from cgreen.

In that case I had come away with a misunderstanding of @d-meiser s comment (#7 (comment)) in issue #7

I had come away with the idea from that discission that we had decided to control the exit vs _exit path with an environment variable (defaulting to _exit) which would also resolve this issue (#6).

in terms of the "run all tests in a single process" approach I personally am not sure (as I commented elsewhere) that it is a viable solution since tests will no longer be sandboxed in their own child process and so users will be beset with problems with shared state (static variables etc) hanging around between tests.

Unless anyone has a better idea: I would like to go ahead as per my impression of what we had decided to do for #7 i.e. control exit vs _exit code path via an environment variable (e.g. CGREEN_COVERAGE=1)

from cgreen.

@gardenia you are correct, that was a thinko on my end. Running the tests in-process is not needed for clients of cgreen to get coverage info, using exit() is enough. And I agree, this should be done at runtime rather than at compile time (I did the latter in #7, which is wrong).

from cgreen.

I would favor an environment variable as @gardenia suggests, like I did for the forthcoming CGREEN_NO_FORK functionality. I'm not sure CGREEN_COVERAGE is the right name, since there's a side effect of console output issues when enabled, but naming isn't critical to me.

Divergent binary distributions of cgreen within a product build is something we should try to avoid -- I have run into those issues before with larger projects across multiple companies and it definitely makes for friction in uptake and ongoing maintenance. (It's also a mistake other C++ unit testing libraries make that I'd like to avoid if reasonably possible.)

from cgreen.

I agree about CGREEN_COVERAGE being a wishy-washy name.

in terms of better naming, here are some definitions of exit vs _exit I found on a forum somewhere:

• exit() flushes io buffers and does some other things like run functions registered by atexit(). exit() invokes _end( )
• _exit() just ends the process without doing that. You call _exit() from the parent process when creating a daemon for example

so if the variable is going to enable the former I guess we could call it something very specific like: CGREEN_CHILD_EXIT_WITH_FLUSH

any other ideas?

the only thing about using such a low-level purpose specific naming is that as a user I might find it odd that to get coverage to work properly I have to set an environment variable with a name so far removed from my goal (of coverage working). but I guess that is of minimal concern and can be solved with cookbook style documentation.

from cgreen.

sketched out the basic patch (leaving @d-meiser ifdef also for now) but I want to be able to run the unit tests with coverage as part of the regression suite to prove the fix works (and continues to).

@d-meiser - in issue #7 you referenced some Travis logs which looked like you had it set up to run with coverage. is this automated? if so where can I see the script / target which runs this. is the scripty part of this checked into the cgreen repo? or does it only exist in some travis config file?

from cgreen.

@gardenia, yes the .travis.yml file is in the repo. If you want I can take care of the necessary changes in that file.

from cgreen.

@gardenia any progress on this? We have coverage setup thanks to @d-meiser and that works. I'm wondering if that would be sufficient? It would be tricky to arrange for a specific coverage regression test case, I think.

I'm not sure how a breakage of this functionality would manifest itself in coveralls though, probably by dropping drastically...

from cgreen.

potentially dumb question: why don't we call some *flush() functions before _exit() to emulate the aspects we need and leave behind the ones we don't want? that may require from platform-specific ifdefs/link-time substitutions in the runner code, but I'm okay with that.

second dumb question: can we just use the AutoFDO functionality in GCC 5.x (and for LLVM/clang by using https://github.com/google/autofdo )? then we wouldn't need to rebuild with profile-generate, everything would run faster, and we can sidestep this for our own tests while giving a working example of how users can do code coverage in a more modern way.

from cgreen.

On 2nd dumb question: You are thinking that if we only profile in a controlled environment, say Travis, we can ensure that AutoFDO (of which I know next to nothing) work there, and that would be sufficient for our own profiling needs? So, no need to ensure profiling works in every environment?

from cgreen.

@matthargett

re: PDQ #1: IIRC that was my first reaction also but when I looked at implementations of exit() in glibc I started to get scared about looking too much behind the curtain. For example, here is a popular implementation of stblib exit

https://github.com/lattera/glibc/blob/a2f34833b1042d5d8eeb263b4cf4caaea138c4ad/stdlib/exit.c

which is really just running a bunch of registered callbacks. I guess we could work out exactly which callbacks are required to get the coverage stuff to flush properly but I became (admittidely perhaps too quickly) put off by the idea by looking at that code and trying to work out which particular system calls are needed

I can't remember if I tried just running flush() then _exiit() but it seems like something I might have tried. Feel free to try it of course. other than that we would need to work out what system calls are needed before _exit() for coverage stuff to also work properly. I guess we could run strace as a quick way to see what happens when exit() runs and narrow it down. It would be an interesting thing to toy around with.

I do think the existing fix apart from the environment variable trade-off is ok or certainly for my use-case

re: PDQ #2: sounds like a good thing to do in any case :)

from cgreen.

As far as I can see the original issue is fixed. I can successfully generate coverage for the unit tests run for an application if CGREEN_CHILD_EXIT_WITH_FLUSH is set. Documentation remains todo before closing this issue.

One last thought, though. We could call _gcov_flush() (see e.g. https://www.osadl.org/Dumping-gcov-data-at-runtime-simple-ex.online-coverage-analysis.0.html and some indications on Stackoverflow) to do what we want before the child exits. But if that requires a rebuild of Cgreenfor client coverage then that's not an option. So if there is a "portable" way to see if an external function is available at run-time we could use that, but I know of no such way.

Anyone got any experience or thoughts?

from cgreen.

we discussed _gcov_flush() briefly earlier in this issues comments.

I think my conclusion was that cgreen itself would need to be linked against gcov to provide that symbol and then it is really just a question of whether we want to require that build time depedency. maybe there is nothing bad about that. I'm not sure. my intution is to try to minimize link time depdencies for non-critical path functionality but I may be over thinking it (a common problem I have :)

I don't know of any portable way to load _gcov_flush() at runtime.

from cgreen.

We wouldn't need to load it, just see if it is loaded and if so, call it. I was thinking something out of dlfcn.h, like

if (handle = dlopen("gcov") && (gcov_flush_p = dlsym("_gcov_flush"))
  gcov_flush_p();

I haven't tested if you can dlopen() without knowing the complete path of the lib, though. That's probably not going to work...

I don't know if there is a runtime (or other) penalty for being linked dynamically with a lib that is not called. However, this would also affect static builds, making them more complex to get working in environments where gcov is not available. So then we'd need a way to build without anyway, which of course is just some more CMake magic.

from cgreen.

There's also difficulty when multiple toolchains are installed. We want to use the gcov library that matches the linked libgcc (assuming it's compiled with GCC and dynamically linked in the first place). This isn't a hypothetical -- 3 different jobs I was at, the developers used 3 different toolchains in Ubuntu: system default toolchain, gcc-snapshot, and a custom-bootstrap GCC inserted into the PATH.

I'm curious how precise AutoFDO gets from a coverage perspective. That's Linux-only, but the strategy could be applied on other OSes and the AutoFDO scripts could be updated to convert those profilers' data formats. Based on my experience, there needs to be a holistic set of options for people to use.

from cgreen.

What if we think about it from the opposite direction....

Given that

• when you want coverage you just want it to work and not need to dig into documentation of isoteric tools ;-)
• the flush-issue with child exit() mostly never appear (I can't even repeat it in the enviroment that I know the problem existed in and you have to have a lot of output for it to appear which is probably rare)

if we use exit() in the child (which formally is incorrect according to POSIX) in the normal case then we get happy and unsurprised gcov'ers. Some users, probably very very few, will encounter strange output problems, google "cgreen garbled output" and find the documentation about how to fix this.

If we think it is a greater risk/problem/setback/adoption hamper(?) that coverage does not work out of the box, than having rare flush-issues, then the solution is to reverse the current fix, let the environment variable indicate use of _exit() and in normal cases use exit().

from cgreen.

I like that idea. in a way that was my original pretext when this bug first came up but I have no first hand experience of the buffered output problem so I was never clear how impactful it is. I like the idea that coverage would work as expected out of the box without having to google for magic incantations. However, on the flip side it may be good to have a "cgreen and coverage" section in the docs anyway as even at the best of times I generally don't remember the relevant C flags or various command-line invocations required to get a coverage report so having one extra caveat in those docs may not be so bad either.

either way it doesn't seem like it would create a breaking incompaibility to change our minds about this at a later date.

from cgreen.

@thoni56 I think that's a great idea. In pretty much every project where I've used cgreen I export CGREEN_CHILD_EXIT_WITH_FLUSH=TRUE, so for me exit() has been the default. This has worked well for me.

from cgreen.

I'm a few years removed from helping devs work with cgreen, so this all sounds reasonable to me :)

from cgreen.

Implemented the latest suggestion as PR #68. Closing this issue. Finally ;-)

from cgreen.