[Bug gcov/profile/28441] New: Need atomic increment of gcov counters for MP programs

[Bug gcov/profile/28441] New: Need atomic increment of gcov counters for MP programs

[gnb at sgi dot com]

The arc instrumentation code generated by -fprofile-arcs increments
gcov counters in a manner which is not guaranteed to be atomic.  As
a result, if two or more cpus enter the same basic block at nearly
the same time they can leave an incorrect arc count, which makes it
impossible for gcov to solve that function's arc count graph later.

Within SGI, this is a significant limitation when doing kernel test
coverage studies, as we have code that needs to be exercised over
multiple processors.  The same problem presumably affects people
trying to do coverage studies on multithreaded programs in userspace.

I've written a patch to solve this problem.  It adds a new option,
-fprofile-multithread, which changes the tree profiler code to
use the recently-added __sync_add_and_fetch() builtin to update
the counters.  I've tested it on ia64 and i386; it generates
the appropriate object code which runs and produces coverage
results.

Here's a diffstat:

 common.opt                         |    4 +
 doc/invoke.texi                    |   14 ++++-
 testsuite/gcc.misc-tests/gcov-12.c |   28 ++++++++++
 tree-profile.c                     |   75 +++++++++++++++++++++++++++++
 4 files changed, 119 insertions(+), 2 deletions(-)

Reading the documention at gcc.gnu.org, this looks "legally significant"
so I'll need to have some paperwork done right?  I assume a copyright
assignment form and an employer disclaimer are the correct documents.
Can someone start that process?  Should I just attach the patch now?


-- 
           Summary: Need atomic increment of gcov counters for MP programs
           Product: gcc
           Version: 4.1.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: gcov/profile
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: gnb at sgi dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2006-07-20 01:09 -------
Patches go to gcc-patches@ anyways.
Also please read http://gcc.gnu.org/contribute.html if you have not already.



Yes this does legally significant.  Oher than I am not a lawyer and this is not
legal advice.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[gnb at sgi dot com]

------- Comment #2 from gnb at sgi dot com  2006-07-20 01:51 -------
Thanks Andrew,

> Also please read http://gcc.gnu.org/contribute.html if you have not already.

I have read that, and while it does mention all the requirements it left
me confused about I should do to start the process.  Should I just submit
the patch to the gcc-patches ML and wait for someone to notice that I
haven't got paperwork?  Or do I start with an email to assignments@gnu.org
then send the patch after completing the paperwork process?

Sorry for the newbie questions.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[bangerth at dealii dot org]

------- Comment #3 from bangerth at dealii dot org  2006-07-20 03:45 -------
That's an important contribution, and I want to encourage you not to get
discouraged by the lengthy process of getting a copyright assignment and
then patch review. Please hang tight, this is a patch that others will
also find of interest!

Best
  Wolfgang


-- 

bangerth at dealii dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bangerth at dealii dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[ian at airs dot com]

------- Comment #4 from ian at airs dot com  2006-07-21 18:05 -------
Please send e-mail to assignments@gnu.org first.  When that process is
complete, send the patch to gcc-patches@gcc.gnu.org.

Thanks!


-- 

ian at airs dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ian at airs dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[seongbae dot park at gmail dot com]

------- Comment #5 from seongbae dot park at gmail dot com  2006-07-23 17:27 -------
It seems that you didn't change libgcov.c,
which suggests that you didn't address __gcov_{pow2,interval}_profiler.
Without such change, -fprofile-generate will cause the mismatch between
the value counters and edge counters, 
so it would be very nice if you can fix that as well 
(this is just a suggestion).

In case someone wants to address that issue,
I think there are three choices:

#1 make above profiler routines to use atomic increment *always*
#2 introduce a new environment variable to pick atomic/non-atomic increment
#3 make atomic increment version of those routines and -fprofile-multithread to
generate codes to call them.

I prefer #3, but #1 might be simple enough without much bad affect.

Another comment is about the name of -fprofile-multithread.
There's an alternative MT-safe profiling scheme of making counters TLS.
So I'd prefer if the option for atomic increment is more explicit, 
something like -fprofile-atomic-increment.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[gnb at sgi dot com]

------- Comment #6 from gnb at sgi dot com  2006-07-24 02:23 -------
Ian Lance Taylor says:
> Please send e-mail to assignments@gnu.org first.  When that process is
> complete, send the patch to gcc-patches@gcc.gnu.org.

Thanks, that was the guidance I needed ;-)  The process is underway.

Seongbae Park says:
> It seems that you didn't change libgcov.c,
> which suggests that you didn't address __gcov_{pow2,interval}_profiler.

That's correct, I haven't addressed those, for the simple
reason that I don't use them and so hadn't noticed an issue.

> so it would be very nice if you can fix that as well 
> (this is just a suggestion).

I'm willing to try ;-) but no promises.

> I think there are three choices:
> 
> #1 make above profiler routines to use atomic increment *always*

I decided not to do this for the -fprofile-arcs case because:

a.  Atomic increment is a new feature of gcc and so I assume it's not
    available on all platforms, even all those whose ISA supports it.

b.  Even when available, atomic increment may incur additional overhead
    which isn't necessary in a single-threaded context, e.g. the mf
    (memory fence) instruction on ia64.

I expect the same arguments apply to -fprofile-values.

> #2 introduce a new environment variable to pick atomic/non-atomic increment

The -fprofile-arcs instrumentation would be hard to make behave this
way without incurring additional runtime overhead, so I think this
approach would just provide an opportunity for the two options to
behave inconsistently.

> #3 make atomic increment version of those routines and
> -fprofile-multithread to generate codes to call them.
>
> I prefer #3, [...]

Agreed.

> Another comment is about the name of -fprofile-multithread.
> There's an alternative MT-safe profiling scheme of making counters TLS.

Yes, actually that was my first approach.  However I couldn't figure
out a way to make it work in the kernel context (need to gather coverage
for dynamically loaded kernel modules), safe (need to aggregate counters
from multiple threads/cpus atomically while the code may be running),
and efficient (avoid one or two indirections).  It would clearly be a
better approach if it could be made to work, because it avoid the problem
of bouncing counter cachelines in large multiprocessor machines such as
SGI makes.  Using the atomic increment builtins is a lesser but easier
solution.

> So I'd prefer if the option for atomic increment is more explicit, 
> something like -fprofile-atomic-increment.

Fair enough, I'll make that change.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[pinskia at gcc dot gnu dot org]

------- Comment #7 from pinskia at gcc dot gnu dot org  2006-07-26 00:28 -------
Confirmed.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2006-07-26 00:28:18
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[steven at gcc dot gnu dot org]

------- Comment #8 from steven at gcc dot gnu dot org  2006-07-29 00:41 -------
Is it necessary to wait with posting until the copyright assignment process is
finished, or can the patch be posted for a first review before the assignment
is on file?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[ian at airs dot com]

------- Comment #9 from ian at airs dot com  2006-07-29 05:21 -------
It is safest to avoid posting the patch to a gcc mailing list before the
copyright assignment is signed.  It protects us in the (hopefully unlikely)
case that the copyright assignment never does get signed.  Otherwise we have a
posted patch but no right to use it.  If that happens, then when we eventually
implement the same functionality in a different way, we are open to accusations
that we have stolen the patch.  Naturally the current SGI, which I presume is
the legal owner of the code, would not do that.  But I'm afraid we have to
consider that there is likely to be a new legal owner of this code in the
relatively near future, and we can not predict what they will do.  This is all
pretty small potatoes, of course, but there is no reason to add an unnecessary
legal risk to the gcc project.

None of this says that Greg could not sent out the patch to individuals,
though, so if you are interested in seeing it, drop him a note and see what he
thinks.

Hope this helps.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov/profile/28441] Need atomic increment of gcov counters for MP programs

[gnb at sgi dot com]

------- Comment #10 from gnb at sgi dot com  2006-07-31 01:18 -------
Ian: understood and agreed.

FYI: the copyright assignment arrived in this morning's mail.
I'll need to run it past my lawyer (as I do any legal document)
but I don't expect that will take long.  The bottleneck is
likely to be the employer disclaimer, which will need to go
back across the Pacific and up through some layers of corporate
bureaucracy.  I assume you need both before I can post?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov-profile/28441] Need atomic increment of gcov counters for MP programs

[gnb at melbourne dot sgi dot com]

------- Comment #11 from gnb at melbourne dot sgi dot com  2006-12-06 20:49 -------
(In reply to comment #10)
> [...] the employer disclaimer, which will need to go
> back across the Pacific and up through some layers of corporate
> bureaucracy.

I'm told this document was signed yesterday, so I expect the
paperwork will be complete in a few days.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov-profile/28441] Need atomic increment of gcov counters for MP programs

[gnb at melbourne dot sgi dot com]

------- Comment #12 from gnb at melbourne dot sgi dot com  2007-01-10 01:25 -------
Today I received a notification from the copyright-clerk:

> Hello Greg,
>                                                                                                                                            
> Your assignment/disclaimer process with the FSF is currently complete;
> please find attached a pdf* copy of the signed form.

I take it I should attach the patch now?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov-profile/28441] Need atomic increment of gcov counters for MP programs

[hubicka at gcc dot gnu dot org]

------- Comment #13 from hubicka at gcc dot gnu dot org  2007-03-14 23:03 -------
Note that Michael Matz commited a patch to solve the problem independently.
http://gcc.gnu.org/ml/gcc-patches/2007-03/msg00950.html
I hope it will get into mainline in few days.

Honza


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov-profile/28441] Need atomic increment of gcov counters for MP programs

[pinskia at gcc dot gnu dot org]

------- Comment #14 from pinskia at gcc dot gnu dot org  2008-08-22 18:16 -------
Any news on the review?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

[Bug gcov-profile/28441] Need atomic increment of gcov counters for MP programs

[steven at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28441

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matz at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org,
                   |                            |steven at gcc dot gnu.org

--- Comment #15 from Steven Bosscher <steven at gcc dot gnu.org> 2012-07-21 23:25:38 UTC ---
Matz's patch http://gcc.gnu.org/ml/gcc-patches/2007-03/msg00950.html was never
committed on the trunk. That's unfortunate, we're living more and more in a
multi-threaded world. Also, the atomic operations in GCC 4.8 should make this
easier to implement for all targets now (instead of only for i386).

Matz, could you have a look and see if you're interested in finishing that nice
piece of work of yours? Or if you're not interested, let that know, too, so
someone else can pick up the ball where you drop it! :-)