* [PATCH] gcov: fix TOPN streaming from shared libraries
@ 2020-09-06 11:24 Sergei Trofimovich
2020-09-21 11:10 ` Martin Liška
0 siblings, 1 reply; 5+ messages in thread
From: Sergei Trofimovich @ 2020-09-06 11:24 UTC (permalink / raw)
To: Jan Hubicka, Martin Liska, Nathan Sidwell, gcc-patches; +Cc: Sergei Trofimovich
From: Sergei Trofimovich <siarheit@google.com>
Before the change gcc did not stream correctly TOPN counters
if counters belonged to a non-local shared object.
As a result zero-section optimization generated TOPN sections
in a form not recognizable by '__gcov_merge_topn'.
The problem happens because in a case of multiple shared objects
'__gcov_merge_topn' function is present in address space multiple
times (once per each object).
The fix is to never rely on function address and predicate on TOPN
counter types.
libgcc/ChangeLog:
PR gcov-profile/96913
* libgcov-driver.c (write_one_data): Avoid function pointer
comparison in TOP streaming decision.
---
libgcc/libgcov-driver.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
index 58914268d4e..86a6b5ad68a 100644
--- a/libgcc/libgcov-driver.c
+++ b/libgcc/libgcov-driver.c
@@ -424,10 +424,15 @@ write_one_data (const struct gcov_info *gi_ptr,
n_counts = ci_ptr->num;
- if (gi_ptr->merge[t_ix] == __gcov_merge_topn)
+ /* Do not zero-compress top counters because:
+ * - __gcv_merge_topn does not handle such sections
+ * - GCOV_COUNTER_V_INDIR contains non-zero keys
+ */
+ if (t_ix == GCOV_COUNTER_V_TOPN || t_ix == GCOV_COUNTER_V_INDIR)
write_top_counters (ci_ptr, t_ix, n_counts);
else
{
+
/* Do not stream when all counters are zero. */
int all_zeros = 1;
for (unsigned i = 0; i < n_counts; i++)
--
2.28.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gcov: fix TOPN streaming from shared libraries
2020-09-06 11:24 [PATCH] gcov: fix TOPN streaming from shared libraries Sergei Trofimovich
@ 2020-09-21 11:10 ` Martin Liška
2020-09-21 17:38 ` Alexander Monakov
0 siblings, 1 reply; 5+ messages in thread
From: Martin Liška @ 2020-09-21 11:10 UTC (permalink / raw)
To: Sergei Trofimovich, Jan Hubicka, Nathan Sidwell, gcc-patches
Cc: Sergei Trofimovich, Alexander Monakov
On 9/6/20 1:24 PM, Sergei Trofimovich wrote:
> From: Sergei Trofimovich <siarheit@google.com>
>
> Before the change gcc did not stream correctly TOPN counters
> if counters belonged to a non-local shared object.
>
> As a result zero-section optimization generated TOPN sections
> in a form not recognizable by '__gcov_merge_topn'.
>
> The problem happens because in a case of multiple shared objects
> '__gcov_merge_topn' function is present in address space multiple
> times (once per each object).
>
> The fix is to never rely on function address and predicate on TOPN
> counter types.
Hello.
Thank you for the analysis! I think it's the correct fix and it's probably
similar to what we used to see for indirect_call_tuple.
@Alexander: Am I right?
Thanks,
Martin
>
> libgcc/ChangeLog:
>
> PR gcov-profile/96913
> * libgcov-driver.c (write_one_data): Avoid function pointer
> comparison in TOP streaming decision.
> ---
> libgcc/libgcov-driver.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
> index 58914268d4e..86a6b5ad68a 100644
> --- a/libgcc/libgcov-driver.c
> +++ b/libgcc/libgcov-driver.c
> @@ -424,10 +424,15 @@ write_one_data (const struct gcov_info *gi_ptr,
>
> n_counts = ci_ptr->num;
>
> - if (gi_ptr->merge[t_ix] == __gcov_merge_topn)
> + /* Do not zero-compress top counters because:
> + * - __gcv_merge_topn does not handle such sections
> + * - GCOV_COUNTER_V_INDIR contains non-zero keys
> + */
> + if (t_ix == GCOV_COUNTER_V_TOPN || t_ix == GCOV_COUNTER_V_INDIR)
> write_top_counters (ci_ptr, t_ix, n_counts);
> else
> {
> +
> /* Do not stream when all counters are zero. */
> int all_zeros = 1;
> for (unsigned i = 0; i < n_counts; i++)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gcov: fix TOPN streaming from shared libraries
2020-09-21 11:10 ` Martin Liška
@ 2020-09-21 17:38 ` Alexander Monakov
2020-09-21 23:13 ` Sergei Trofimovich
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Monakov @ 2020-09-21 17:38 UTC (permalink / raw)
To: Martin Liška
Cc: Sergei Trofimovich, Jan Hubicka, Nathan Sidwell, gcc-patches,
Sergei Trofimovich
On Mon, 21 Sep 2020, Martin Liška wrote:
> On 9/6/20 1:24 PM, Sergei Trofimovich wrote:
> > From: Sergei Trofimovich <siarheit@google.com>
> >
> > Before the change gcc did not stream correctly TOPN counters
> > if counters belonged to a non-local shared object.
> >
> > As a result zero-section optimization generated TOPN sections
> > in a form not recognizable by '__gcov_merge_topn'.
> >
> > The problem happens because in a case of multiple shared objects
> > '__gcov_merge_topn' function is present in address space multiple
> > times (once per each object).
> >
> > The fix is to never rely on function address and predicate on TOPN
> > counter types.
>
> Hello.
>
> Thank you for the analysis! I think it's the correct fix and it's probably
> similar to what we used to see for indirect_call_tuple.
>
> @Alexander: Am I right?
Yes, analysis presented by Sergei in Bugzilla looks correct. Pedantically I
wouldn't say the indirect call issue was similar: it's a different gotcha
arising from mixing static and dynamic linking. There we had some symbols
preempted by the main executable (but not all symbols), here we have lack
of preemption/unification as relevant libgcov symbol is hidden.
I cannot judge if the fix is correct (don't know the code that well) but it
looks reasonable. If you could come up with a clearer wording for the new
comment it would be nice, I struggled to understand it.
Thanks.
Alexander
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gcov: fix TOPN streaming from shared libraries
2020-09-21 17:38 ` Alexander Monakov
@ 2020-09-21 23:13 ` Sergei Trofimovich
2020-09-22 7:12 ` Martin Liška
0 siblings, 1 reply; 5+ messages in thread
From: Sergei Trofimovich @ 2020-09-21 23:13 UTC (permalink / raw)
To: Alexander Monakov
Cc: Martin Liška, Jan Hubicka, Nathan Sidwell, gcc-patches,
Sergei Trofimovich
[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]
On Mon, 21 Sep 2020 20:38:07 +0300 (MSK)
Alexander Monakov <amonakov@ispras.ru> wrote:
> On Mon, 21 Sep 2020, Martin Liška wrote:
>
> > On 9/6/20 1:24 PM, Sergei Trofimovich wrote:
> > > From: Sergei Trofimovich <siarheit@google.com>
> > >
> > > Before the change gcc did not stream correctly TOPN counters
> > > if counters belonged to a non-local shared object.
> > >
> > > As a result zero-section optimization generated TOPN sections
> > > in a form not recognizable by '__gcov_merge_topn'.
> > >
> > > The problem happens because in a case of multiple shared objects
> > > '__gcov_merge_topn' function is present in address space multiple
> > > times (once per each object).
> > >
> > > The fix is to never rely on function address and predicate on TOPN
> > > counter types.
> >
> > Hello.
> >
> > Thank you for the analysis! I think it's the correct fix and it's probably
> > similar to what we used to see for indirect_call_tuple.
> >
> > @Alexander: Am I right?
>
> Yes, analysis presented by Sergei in Bugzilla looks correct. Pedantically I
> wouldn't say the indirect call issue was similar: it's a different gotcha
> arising from mixing static and dynamic linking. There we had some symbols
> preempted by the main executable (but not all symbols), here we have lack
> of preemption/unification as relevant libgcov symbol is hidden.
>
> I cannot judge if the fix is correct (don't know the code that well) but it
> looks reasonable. If you could come up with a clearer wording for the new
> comment it would be nice, I struggled to understand it.
Yeah, I agree the comment is very misleading. The code is already very clear
about special casing of TOPN counters. How about dropping the comment?
v2:
From 300585164f0a719a3a283c8da3a4061615f6da3a Mon Sep 17 00:00:00 2001
From: Sergei Trofimovich <siarheit@google.com>
Date: Sun, 6 Sep 2020 12:13:54 +0100
Subject: [PATCH v2] gcov: fix TOPN streaming from shared libraries
Before the change gcc did not stream correctly TOPN counters
if counters belonged to a non-local shared object.
As a result zero-section optimization generated TOPN sections
in a form not recognizable by '__gcov_merge_topn'.
The problem happens because in a case of multiple shared objects
'__gcov_merge_topn' function is present in address space multiple
times (once per each object).
The fix is to never rely on function address and predicate on TOPN
counter types.
libgcc/ChangeLog:
PR gcov-profile/96913
* libgcov-driver.c (write_one_data): Avoid function pointer
comparison in TOP streaming decision.
---
libgcc/libgcov-driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
index 58914268d4e..e53e4dc392a 100644
--- a/libgcc/libgcov-driver.c
+++ b/libgcc/libgcov-driver.c
@@ -424,7 +424,7 @@ write_one_data (const struct gcov_info *gi_ptr,
n_counts = ci_ptr->num;
- if (gi_ptr->merge[t_ix] == __gcov_merge_topn)
+ if (t_ix == GCOV_COUNTER_V_TOPN || t_ix == GCOV_COUNTER_V_INDIR)
write_top_counters (ci_ptr, t_ix, n_counts);
else
{
--
Sergei
[-- Attachment #2: v2-0001-gcov-fix-TOPN-streaming-from-shared-libraries.patch --]
[-- Type: text/x-patch, Size: 1381 bytes --]
From 300585164f0a719a3a283c8da3a4061615f6da3a Mon Sep 17 00:00:00 2001
From: Sergei Trofimovich <siarheit@google.com>
Date: Sun, 6 Sep 2020 12:13:54 +0100
Subject: [PATCH v2] gcov: fix TOPN streaming from shared libraries
Before the change gcc did not stream correctly TOPN counters
if counters belonged to a non-local shared object.
As a result zero-section optimization generated TOPN sections
in a form not recognizable by '__gcov_merge_topn'.
The problem happens because in a case of multiple shared objects
'__gcov_merge_topn' function is present in address space multiple
times (once per each object).
The fix is to never rely on function address and predicate on TOPN
counter types.
libgcc/ChangeLog:
PR gcov-profile/96913
* libgcov-driver.c (write_one_data): Avoid function pointer
comparison in TOP streaming decision.
---
libgcc/libgcov-driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
index 58914268d4e..e53e4dc392a 100644
--- a/libgcc/libgcov-driver.c
+++ b/libgcc/libgcov-driver.c
@@ -424,7 +424,7 @@ write_one_data (const struct gcov_info *gi_ptr,
n_counts = ci_ptr->num;
- if (gi_ptr->merge[t_ix] == __gcov_merge_topn)
+ if (t_ix == GCOV_COUNTER_V_TOPN || t_ix == GCOV_COUNTER_V_INDIR)
write_top_counters (ci_ptr, t_ix, n_counts);
else
{
--
2.28.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gcov: fix TOPN streaming from shared libraries
2020-09-21 23:13 ` Sergei Trofimovich
@ 2020-09-22 7:12 ` Martin Liška
0 siblings, 0 replies; 5+ messages in thread
From: Martin Liška @ 2020-09-22 7:12 UTC (permalink / raw)
To: Sergei Trofimovich, Alexander Monakov
Cc: Jan Hubicka, Nathan Sidwell, gcc-patches, Sergei Trofimovich
On 9/22/20 1:13 AM, Sergei Trofimovich wrote:
> On Mon, 21 Sep 2020 20:38:07 +0300 (MSK)
> Alexander Monakov <amonakov@ispras.ru> wrote:
>
>> On Mon, 21 Sep 2020, Martin Liška wrote:
>>
>>> On 9/6/20 1:24 PM, Sergei Trofimovich wrote:
>>>> From: Sergei Trofimovich <siarheit@google.com>
>>>>
>>>> Before the change gcc did not stream correctly TOPN counters
>>>> if counters belonged to a non-local shared object.
>>>>
>>>> As a result zero-section optimization generated TOPN sections
>>>> in a form not recognizable by '__gcov_merge_topn'.
>>>>
>>>> The problem happens because in a case of multiple shared objects
>>>> '__gcov_merge_topn' function is present in address space multiple
>>>> times (once per each object).
>>>>
>>>> The fix is to never rely on function address and predicate on TOPN
>>>> counter types.
>>>
>>> Hello.
>>>
>>> Thank you for the analysis! I think it's the correct fix and it's probably
>>> similar to what we used to see for indirect_call_tuple.
>>>
>>> @Alexander: Am I right?
>>
>> Yes, analysis presented by Sergei in Bugzilla looks correct. Pedantically I
>> wouldn't say the indirect call issue was similar: it's a different gotcha
>> arising from mixing static and dynamic linking. There we had some symbols
>> preempted by the main executable (but not all symbols), here we have lack
>> of preemption/unification as relevant libgcov symbol is hidden.
Thank you Alexander.
>>
>> I cannot judge if the fix is correct (don't know the code that well) but it
>> looks reasonable. If you could come up with a clearer wording for the new
>> comment it would be nice, I struggled to understand it.
>
> Yeah, I agree the comment is very misleading. The code is already very clear
> about special casing of TOPN counters. How about dropping the comment?
>
> v2:
>
> From 300585164f0a719a3a283c8da3a4061615f6da3a Mon Sep 17 00:00:00 2001
> From: Sergei Trofimovich <siarheit@google.com>
> Date: Sun, 6 Sep 2020 12:13:54 +0100
> Subject: [PATCH v2] gcov: fix TOPN streaming from shared libraries
>
> Before the change gcc did not stream correctly TOPN counters
> if counters belonged to a non-local shared object.
>
> As a result zero-section optimization generated TOPN sections
> in a form not recognizable by '__gcov_merge_topn'.
>
> The problem happens because in a case of multiple shared objects
> '__gcov_merge_topn' function is present in address space multiple
> times (once per each object).
>
> The fix is to never rely on function address and predicate on TOPN
> counter types.
>
> libgcc/ChangeLog:
>
> PR gcov-profile/96913
> * libgcov-driver.c (write_one_data): Avoid function pointer
> comparison in TOP streaming decision.
> ---
> libgcc/libgcov-driver.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
> index 58914268d4e..e53e4dc392a 100644
> --- a/libgcc/libgcov-driver.c
> +++ b/libgcc/libgcov-driver.c
> @@ -424,7 +424,7 @@ write_one_data (const struct gcov_info *gi_ptr,
>
> n_counts = ci_ptr->num;
>
> - if (gi_ptr->merge[t_ix] == __gcov_merge_topn)
> + if (t_ix == GCOV_COUNTER_V_TOPN || t_ix == GCOV_COUNTER_V_INDIR)
> write_top_counters (ci_ptr, t_ix, n_counts);
> else
> {
>
The fix is fine, please install it.
Martin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-22 7:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-06 11:24 [PATCH] gcov: fix TOPN streaming from shared libraries Sergei Trofimovich
2020-09-21 11:10 ` Martin Liška
2020-09-21 17:38 ` Alexander Monakov
2020-09-21 23:13 ` Sergei Trofimovich
2020-09-22 7:12 ` Martin Liška
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).