Nathan, I just noticed this path. This is a good improvement over the existing scheme.
I see one potential problem with the patch -- different instances of the same comdat function can potentially have different control flows (due to differences in early inline, early optimizations in different module contexts) before profile counter allocation -- which means making the profile counters 'comdat' can later cause coverage mismatch problems during profile-use phase. In this patch, fn data buffer is introduced. What is that for and how does it work? thanks, David On Wed, Nov 2, 2011 at 1:18 PM, Nathan Sidwell <nat...@acm.org> wrote: > This patch modifies the coverage scheme so that it works better with comdat > sections. The existing scheme uses a single file-scope array for counters, > with each function using a chunk of that array. If the function is comdat, > it's possible that that instance will be omitted from the link, in > preference to one from another object file. The chunk of the counter array > will remain and remain as zero counts. When processed by gcov, that > instance of the function will appear unexecuted, which is misleading, > because it's not present at all in the final executable. For C, this > affects unlined inline functions, for C++ it also affects template > instantiations. > > This patch changes things so that counters are allocated as per-function > arrays with the same comdatedness as the function being instrumented. Thus > the array will be discarded, if the function itself is. Each function's > function-descriptor now also points to the counter array, in addition to > specifying the number of counters instrumented (plus the existing > checksums). This object too has the same comdatedness (& visibility) as the > function. The name of this object is derived from the name of the function, > via a mangling scheme. > > In the existing scheme, there's a global file-scope object-descriptor that > (a) points to the global counter array, and (b) has a trailing array of > function-descriptors. The patch changes things so that there's no pointer > to the now-deleted global counter array, and the trailing array is now an > array of pointers to function-descriptors. In the absence of comdat, we can > now generate the same gcov data file as before, but with an additional layer > of indirection. > > However, with comdat functions the function-descriptor selected for a > particular function may come from a different object file. It'll be pointed > to by the object-descriptor's trailing array. When iterating over this > array we do not want to emit data for the instance of the function being > pointed to, and so need some way of detecting a foreign function descriptor. > We do this by adding a key field to the function descriptor. It is > initialized to point to the (static) object-descriptor. Thus foreign > function-descriptors will have a key that does not point to the > object-descriptor being iterated over. > > The above description only discusses the block counters. The coverage > system instruments more than that, and each distinct counter type is > modified in the same way. One significant change is that the merge function > pointers are no longer per-function-descriptor, but are in the > object-descriptor and a NULL value there indicates a skipped counter. > > For the data-file itself, the per-object summary data is deleted, as it no > longer makes much sense. The number of instrumented functions from an > object file can vary, depending on which comdat functions are selected from > it in the final links it participates in. For convenience the program > summaries are now placed before the per-function data. Also, because an > object can be in multiple executables (or shared objects), we need to keep > track of functions that were emitted in the object file but not in the final > link. Thus we now have place-holder function data slots, and have the > ability to buffer file data for a function that was instrumented in some > other executable but not in the current one -- I couldn't figure out a sane > way to deal this other than using malloc. I have not removed gcov-dump > support for the per-object summary, to allow use with older data files. But > remember the gcov file format is explicitly tied to the version of the > compiler used to generate it -- it is not supposed to be version neutral. > > As mentioned above, there is an ABI impact, because the function-descriptors > of externally visible functions must also be externally visible. These are > named '__gcov__FUNCTION', where 'FUNCTION' is the (possibly mangled) name of > the function being described. The per-function counter arrays are not > externally visible, so their mangling only needs to guarantee uniqueness in > the object file. These are mangled as '__gcovN_FUNCTION', where 'N' is the > counter number. > > There are a couple of other changes, due to existing shortfalls exposed by > this work. Firstly, for an inline function that is inlined everywhere, we > (currently) generate a block of counters in the global array. We never emit > the non-inlined body of function, so these ended up as unused parts of the > global array. When emitting the object-descriptor, we elide functions whose > DECL_STRUCT_FUNCTION has been cleared. The impact of this in reading the > count data file is that we no longer emit a diagnostic for functions that > are not described in the data file -- we don't know at this point whether > the function being optimized will actually be emitted. > > Tested by normal and profiled bootstrap on i686-pc-linux-gnu for all > languages. > > I'll apply this patch shortly, unless there are questions or comments. > > nathan
