Looks like the change to put fn_info and counter vars into comdat section is reverted due to regression.
Assuming the comdat changes are kept, I don't see the point of doing fn data buffering -- it is certainly important to compare the key with the current gcov_info to avoid merging to happen multiple times for the same comdat function, but there seems no point to do the buffering -- just need to read the profile data and discard (for file seeking). The buffering is also wrong in the sense that it basically skips the merging for the function instance in modules other than the owning module (the final defining module of the comdat). Am I missing something? thanks, David On Fri, Jan 20, 2012 at 2:41 PM, Xinliang David Li <davi...@google.com> wrote: > 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
