On 2/18/21 11:02 AM, Richard Biener wrote:
On Thu, Feb 18, 2021 at 10:46 AM Martin Liška <mli...@suse.cz> wrote:
On 2/18/21 10:31 AM, Richard Biener wrote:
On Wed, Feb 17, 2021 at 2:16 PM Martin Liška <mli...@suse.cz> wrote:
On 2/17/21 9:36 AM, Martin Liška wrote:
I'll write it even more robust...
This is more elegant approach I've just tested on the instrumented clang.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
Isn't it still too complicated? We're asked to write N counters so why
don't we just write N counters?
Well, we are asked to stream N counters. But each has a variable length,
which makes it funny :)
Thus, you already do
for (struct gcov_kvp *node = (struct gcov_kvp *)(intptr_t)start;
- node != NULL; node = node->next)
+ node != NULL; node = node->next, j++)
{
gcov_write_counter (node->value);
gcov_write_counter (node->count);
+
+ /* Stop when we reach expected number of items. */
+ if (j + 1 == list_sizes[i])
+ break;
}
why isn't this then only thing you need (just using pair_count as gathered
previously)? And just count pair_count to zero as IV? No need to
do the node != NULL check?
We can't do that, because that will lead in a corrupted profile.
We can have 2 problematic situations:
1) linked list is shorter, e.g.
10 2 10000 10
Here 2 represents 2 tuples, but we stream only one {10000: 10}. That happens in
the instrumented clang.
2) linked list is longer, e.g.
10 1 10000 5 222222 5
Here 1 represents 1 tuple, be we stream 2 tuples {10000: 10} and {222222: 5}.
I guess the problem is that for whatever reason the stream includes
unsigned disk_size = GCOV_TOPN_DISK_COUNTERS * counters + 2 * pair_total;
gcov_write_tag_length (GCOV_TAG_FOR_COUNTER (t_ix),
GCOV_TAG_COUNTER_LENGTH (disk_size));
which has the sum of the list lengths, but ...
Yes, GCOV tags always contain size and it's very handy in verification
that a .gcda is in a consistent state.
- gcov_type pair_count = ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 1];
gcov_write_counter (ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i]);
- gcov_write_counter (pair_count);
we stream something like that again. What are the two different counters?
You save one, but the other you take async again?
ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 0] == total_number_of_executions
ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 1] == length of the linked list
ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 2] == pointer to the first item
in a linked list
The disk format here is sub-optimal at least and optimizing the read side
which has locking in place for making the write-side racy doesn't look good.
Yes, we slighly miss some space. Per-file locking should not block streaming
parallel of profiles.
Note architectures with less nice memory ordering guarantees might
eventually see partially updated pointers and counters so I think
we at least want atomic_read ()s of the values with the weakest
consistency possible. (but that can be done as followup if we agree on that)
Can we really see a partially updated pointer?
Not on x86 for aligned data. Note the same applies to the counter values.
We've seen allocating memory from libgcov is problematic, so this is why I'm
asking.
Sure.
Martin
Thanks,
Martin
Richard.
Thanks,
Martin
