Re: distinguishing gcc compilation valgrind false positives
Hi,
On Wed, Nov 24 2021, Jan Hubicka via Gcc wrote:
>> ==5404== Conditional jump or move depends on uninitialised value(s)
>> ==5404== at 0x25DAAD7: incorporate_penalties (ipa-cp.c:3282)
>> ==5404== by 0x25DAAD7: good_cloning_opportunity_p(cgraph_node*, sreal,
>> sreal, profile_count, int) (ipa-cp.c:3340)
>
> I looked at this one (since it is in code I am familiar with) and I
> think it may be real bug. There are flags node_is_self_scc which does
> not seem to be initialized in the constructor.
>
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index 42842d9466a..1d0c115465c 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -623,8 +623,8 @@ ipa_node_params::ipa_node_params ()
> : descriptors (NULL), lattices (NULL), ipcp_orig_node (NULL),
>known_csts (vNULL), known_contexts (vNULL), analysis_done (0),
>node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone
> (0),
> - node_dead (0), node_within_scc (0), node_calling_single_call (0),
> - versionable (0)
> + node_dead (0), node_within_scc (0), node_is_self_scc (0),
> + node_calling_single_call (0), versionable (0)
> {
> }
Oops, can you please commit the change to master and all active
branches?
Thanks,
Martin
Re: distinguishing gcc compilation valgrind false positives
> >
> > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> > index 42842d9466a..1d0c115465c 100644
> > --- a/gcc/ipa-prop.h
> > +++ b/gcc/ipa-prop.h
> > @@ -623,8 +623,8 @@ ipa_node_params::ipa_node_params ()
> > : descriptors (NULL), lattices (NULL), ipcp_orig_node (NULL),
> >known_csts (vNULL), known_contexts (vNULL), analysis_done (0),
> >node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone
> > (0),
> > - node_dead (0), node_within_scc (0), node_calling_single_call (0),
> > - versionable (0)
> > + node_dead (0), node_within_scc (0), node_is_self_scc (0),
> > + node_calling_single_call (0), versionable (0)
> > {
> > }
>
> Oops, can you please commit the change to master and all active
> branches?
OK. To be honest I am not 100% sure if the flag is not always
initialized later.
Zdenek, can you, please, check if the undefined warning in ipcp goes
away with this patch and if so, post what are the remaining ones?
>
> Thanks,
>
> Martin
>
Re: distinguishing gcc compilation valgrind false positives
Hello Jan,
-- Původní e-mail --
Od: Jan Hubicka via Gcc
Komu: Martin Jambor
Datum: 25. 11. 2021 11:13:33
Předmět: Re: distinguishing gcc compilation valgrind false positives
> >
> > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> > index 42842d9466a..1d0c115465c 100644
> > --- a/gcc/ipa-prop.h
> > +++ b/gcc/ipa-prop.h
> > @@ -623,8 +623,8 @@ ipa_node_params::ipa_node_params ()
> > : descriptors (NULL), lattices (NULL), ipcp_orig_node (NULL),
> >known_csts (vNULL), known_contexts (vNULL), analysis_done (0),
> >node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone
> > (0),
> > - node_dead (0), node_within_scc (0), node_calling_single_call (0),
> > - versionable (0)
> > + node_dead (0), node_within_scc (0), node_is_self_scc (0),
> > + node_calling_single_call (0), versionable (0)
> > {
> > }
>
> Oops, can you please commit the change to master and all active
> branches?
> OK. To be honest I am not 100% sure if the flag is not always
> initialized later.
> Zdenek, can you, please, check if the undefined warning in ipcp goes
> away with this patch and if so, post what are the remaining ones?
I can confirm that zero-initializing node_is_self_scc prevents the
uninitialised use warnings in incorporate_penalties (ipa-cp.c:3282)
Thanks,
Zdenek
Re: distinguishing gcc compilation valgrind false positives
> > I can confirm that zero-initializing node_is_self_scc prevents the > uninitialised use warnings in incorporate_penalties (ipa-cp.c:3282) Great, I will commit the patch. But I also wonder if there are any remaining unitialized warnings in ipa code? Honza > > Thanks, > Zdenek > >
Re: distinguishing gcc compilation valgrind false positives
Hello Jan, -- Původní e-mail -- Od: Jan Hubicka Komu: Zdenek Sojka Datum: 25. 11. 2021 12:54:00 Předmět: Re: distinguishing gcc compilation valgrind false positives > > I can confirm that zero-initializing node_is_self_scc prevents the > uninitialised use warnings in incorporate_penalties (ipa-cp.c:3282) > Great, I will commit the patch. But I also wonder if there are any > remaining unitialized warnings in ipa code? (I am sorry for the broken formatting, Seznam web email client is very unfriedly to non-HTML emails) Apart from: ==22707== Invalid read of size 4 ==22707==at 0x197C260: put (hash-map.h:179) which is already reported as https://gcc.gnu.org/PR101292 , the other warnings are still there: ==22707== Invalid read of size 8 ==22707==at 0x13FFBCE: hash_map, tree_node*> >::put(tree_node* const&, tree_node* const&) [clone .isra.0] (hash-map.h:176) ==22707==by 0x1400E76: ipa_param_body_adjustments::prepare_debug_expressions(tree_node*) (ipa-param-manipulation.c:1250) ==22707==by 0x140069B: ipa_param_body_adjustments::prepare_debug_expressions(tree_node*) (ipa-param-manipulation.c:1230) ==22707==by 0x1401997: ipa_param_body_adjustments::common_initialization(tree_node*, tree_node**, vec*) (ipa-param-manipulation.c:1428) ==22707==by 0x16C7AD4: tree_function_versioning(tree_node*, tree_node*, vec*, ipa_param_adjustments*, bool, bitmap_head*, basic_block_def*) (tree-inline.c:6303) ==22707==by 0x117328D: cgraph_node::materialize_clone() (cgraphclones.c:1142) ==22707==by 0x1161A15: cgraph_node::get_untransformed_body() (cgraph.c:3965) ==22707==by 0x13BE3AB: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:720) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:715) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:715) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:715) ==22707==by 0x13C02AB: inline_transform(cgraph_node*) (ipa-inline-transform.c:777) ==22707== Address 0x81c5f98 is 40 bytes inside a block of size 208 free'd ==22707==at 0x484240F: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==22707==by 0x13FFC08: find_slot_with_hash (hash-table.h:967) ==22707==by 0x13FFC08: hash_map, tree_node*> >::put(tree_node* const&, tree_node* const&) [clone .isra.0] (hash-map.h:170) ==22707==by 0x1400E76: ipa_param_body_adjustments::prepare_debug_expressions(tree_node*) (ipa-param-manipulation.c:1250) ==22707==by 0x140069B: ipa_param_body_adjustments::prepare_debug_expressions(tree_node*) (ipa-param-manipulation.c:1230) ==22707==by 0x1401997: ipa_param_body_adjustments::common_initialization(tree_node*, tree_node**, vec*) (ipa-param-manipulation.c:1428) ==22707==by 0x16C7AD4: tree_function_versioning(tree_node*, tree_node*, vec*, ipa_param_adjustments*, bool, bitmap_head*, basic_block_def*) (tree-inline.c:6303) ==22707==by 0x117328D: cgraph_node::materialize_clone() (cgraphclones.c:1142) ==22707==by 0x1161A15: cgraph_node::get_untransformed_body() (cgraph.c:3965) ==22707==by 0x13BE3AB: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:720) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:715) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:715) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:715) ==22707== Block was alloc'd at ==22707==at 0x4844C0F: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==22707==by 0x2803A14: xcalloc (xmalloc.c:164) ==22707==by 0x100DE26: data_alloc (hash-table.h:275) ==22707==by 0x100DE26: alloc_entries (hash-table.h:711) ==22707==by 0x100DE26: hash_table, tree_node*> >::hash_entry, false, xcallocator>::hash_table(unsigned long, bool, bool, bool, mem_alloc_origin) (hash-table.h:628) ==22707==by 0x140271A: hash_map (hash-map.h:142) ==22707==by 0x140271A: ipa_param_body_adjustments::ipa_param_body_adjustments(ipa_param_adjustments*, tree_node*, tree_node*, copy_body_data*, tree_node**, vec*) (ipa-param-manipulation.c:1483) ==22707==by 0x16C7AD4: tree_function_versioning(tree_node*, tree_node*, vec*, ipa_param_adjustments*, bool, bitmap_head*, basic_block_def*) (tree-inline.c:6303) ==22707==by 0x117328D: cgraph_node::materialize_clone() (cgraphclones.c:1142) ==22707==by 0x1161A15: cgraph_node::get_untransformed_body() (cgraph.c:3965) ==22707==by 0x13BE3AB: maybe_materialize_called_clones(cgraph_node*) [clone .isra.0] (ipa-inline-transform.c:720) ==22707==by 0x13BE3DC: maybe_materialize_called_clones(cgrap
Re: distinguishing gcc compilation valgrind false positives
On 11/25/21 13:00, Zdenek Sojka via Gcc wrote: which is already reported ashttps://gcc.gnu.org/PR101292 , the other warnings are still there: Hello. Please create a bugzilla entry for this. Thanks, Martin
Re: [EXTERNAL] Re: Question about match.pd
> (A << B) eq/ne 0
Yes that is correct. But for detecting such pattern you You have to detect B
and make sure B is boolean. GIMPLE transfers that Boolean to integer before
shifting.
After many hours of debugging, I think I managed to find out what is going on.
+/* cmp : ==, != */
+/* ((B0 << x) cmp 0) -> B0 cmp 0 */
+(for cmp (eq ne)
+ (simplify
+ (cmp (lshift (convert@3 boolean_valued_p@0) @1) integer_zerop@2)
+ (if (TREE_CODE (TREE_TYPE (@3)) == INTEGER_TYPE
+ && (GIMPLE || !TREE_SIDE_EFFECTS (@1)))
+(cmp @0 @2
So when I am transforming something like above pattern to (cmp @0 @2) there is
a type mismatch between @0 and @2.
@0 is boolean and @2 is integer. That type mismatch does cause a lot of
headache when going through resimplification.
Best wishes,
Navid.
From: Jeff Law
Sent: Wednesday, November 24, 2021 15:11
To: Navid Rahimi; gcc@gcc.gnu.org
Subject: [EXTERNAL] Re: Question about match.pd
On 11/24/2021 2:19 PM, Navid Rahimi via Gcc wrote:
> Hi GCC community,
>
> I have a question about pattern matching in match.pd.
>
> So I have a pattern like this [1]:
> #define CMP !=
> bool f(bool c, int i) { return (c << i) CMP 0; }
> bool g(bool c, int i) { return c CMP 0;}
>
> It is verifiably correct to transfer f to g [2]. Although this pattern looks
> simple, but the problem rises because GIMPLE converts booleans to int before
> "<<" operation.
> So at the end you have boolean->integer->boolean conversion and the shift
> will happen on the integer in the middle.
>
> For example, for something like:
>
> bool g(bool c){return (c << 22);}
>
> The GIMPLE is:
> _Bool g (_Bool c)
> {
>int _1;
>int _2;
>_Bool _4;
>
> [local count: 1073741824]:
>_1 = (int) c_3(D);
>_2 = _1 << 22;
>_4 = _2 != 0;
>return _4;
> }
>
> I wrote a patch to fix this problem in match.pd:
>
> +(match boolean_valued_p
> + @0
> + (if (TREE_CODE (type) == BOOLEAN_TYPE
> + && TYPE_PRECISION (type) == 1)))
> +(for op (tcc_comparison truth_and truth_andif truth_or truth_orif truth_xor)
> + (match boolean_valued_p
> + (op @0 @1)))
> +(match boolean_valued_p
> + (truth_not @0))
>
> +/* cmp : ==, != */
> +/* ((B0 << x) cmp 0) -> B0 cmp 0 */
> +(for cmp (eq ne)
> + (simplify
> + (cmp (lshift (convert@3 boolean_valued_p@0) @1) integer_zerop@2)
> + (if (TREE_CODE (TREE_TYPE (@3)) == INTEGER_TYPE
> + && (GIMPLE || !TREE_SIDE_EFFECTS (@1)))
> +(cmp @0 @2
>
>
> But the problem is I am not able to restrict to the cases I am interested in.
> There are many hits in other libraries I have tried compiling with
> trunk+patch.
>
> Any feedback?
>
> 1)
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fbugzilla%2Fshow_bug.cgi%3Fid%3D98956&data=04%7C01%7Cnavidrahimi%40microsoft.com%7Caa8c9c8213a245c7ae9d08d9af9fc8ae%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637733923073627850%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=25KlLcsftTmN83rVawoKKaTPJdCdFlmtXMj%2BwsrKWbo%3D&reserved=0
> 2)
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Falive2.llvm.org%2Fce%2Fz%2FUUTJ_v&data=04%7C01%7Cnavidrahimi%40microsoft.com%7Caa8c9c8213a245c7ae9d08d9af9fc8ae%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637733923073637846%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=fwN9%2BB0VObPyuUS2fOtj14i%2BHJIiRhyyjZM4LOF4AP8%3D&reserved=0
It would help to also see the cases you're triggering that you do not
want to trigger.
Could we think of the optimization opportunity in a different way?
(A << B) eq/ne 0 -> A eq/ne (0U >> B)
And I would expect the 0U >> B to get simplified to 0.
Would looking at things that way help?
jeff
gcc-9-20211125 is now available
Snapshot gcc-9-20211125 is now available on https://gcc.gnu.org/pub/gcc/snapshots/9-20211125/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 9 git branch with the following options: git://gcc.gnu.org/git/gcc.git branch releases/gcc-9 revision 3d1f5e86fb4351a109d45fe441b1b00d6e56c277 You'll find: gcc-9-20211125.tar.xzComplete GCC SHA256=8e9f79a98e8fffa14dc98ea6731b2e18fb7016a36f4d21d28d5e81575ffdcee2 SHA1=6ff9b4788c37ab0ae4640116f7103b5304564f72 Diffs from 9-2028 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-9 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
Re: atomic_load
Am Sonntag, den 07.11.2021, 10:08 +0100 schrieb Martin Uecker: > It would be great if somebody could take a look at > PR96159. > > It seems we do not do atomic accesses correctly > when the alignment is insufficient for a lockfree > access, but I think we should fall back to a > library call in this case (as clang does). > > This is very unfortunate as it is an important > functionality to be able to do atomic accesses > on non-atomic types and it seems there is no way > to achieve this. > > Also documentation and various descriptions of > the atomic functions imply that this is expected > to work. > > But maybe I am missing something and the generated > code is indeed safe. > > Martin > Could this bug be confirmed please? This is a silent and dangerous incorrect code generation issue. If these functions are not meant to be used to exising data, then at least the documentation needs to be changed and include a big warning that this only happens to work corectly if the data has sufficient alignment for the specific architecture (which of course makes it impossible to use this in a portable way). I would then propose to add atomic_load_safe, so that it is possible to use such functionality safely on existing data structures which is an important use case. Martin
Re: [EXTERNAL] Re: Question about match.pd
On Thu, Nov 25, 2021 at 10:40 PM Navid Rahimi via Gcc wrote:
>
> > (A << B) eq/ne 0
> Yes that is correct. But for detecting such pattern you You have to detect B
> and make sure B is boolean. GIMPLE transfers that Boolean to integer before
> shifting.
Note it's the C language specification that requires this.
> After many hours of debugging, I think I managed to find out what is going on.
>
> +/* cmp : ==, != */
> +/* ((B0 << x) cmp 0) -> B0 cmp 0 */
> +(for cmp (eq ne)
> + (simplify
> + (cmp (lshift (convert@3 boolean_valued_p@0) @1) integer_zerop@2)
> + (if (TREE_CODE (TREE_TYPE (@3)) == INTEGER_TYPE
> + && (GIMPLE || !TREE_SIDE_EFFECTS (@1)))
> +(cmp @0 @2
>
> So when I am transforming something like above pattern to (cmp @0 @2) there
> is a type mismatch between @0 and @2.
> @0 is boolean and @2 is integer. That type mismatch does cause a lot of
> headache when going through resimplification.
Yeah, guess you need
(cmp @0 { build_zero_cst (TREE_TYPE (@0); })
here.
>
>
> Best wishes,
> Navid.
>
>
> From: Jeff Law
> Sent: Wednesday, November 24, 2021 15:11
> To: Navid Rahimi; gcc@gcc.gnu.org
> Subject: [EXTERNAL] Re: Question about match.pd
>
>
>
> On 11/24/2021 2:19 PM, Navid Rahimi via Gcc wrote:
> > Hi GCC community,
> >
> > I have a question about pattern matching in match.pd.
> >
> > So I have a pattern like this [1]:
> > #define CMP !=
> > bool f(bool c, int i) { return (c << i) CMP 0; }
> > bool g(bool c, int i) { return c CMP 0;}
> >
> > It is verifiably correct to transfer f to g [2]. Although this pattern
> > looks simple, but the problem rises because GIMPLE converts booleans to int
> > before "<<" operation.
> > So at the end you have boolean->integer->boolean conversion and the shift
> > will happen on the integer in the middle.
> >
> > For example, for something like:
> >
> > bool g(bool c){return (c << 22);}
> >
> > The GIMPLE is:
> > _Bool g (_Bool c)
> > {
> >int _1;
> >int _2;
> >_Bool _4;
> >
> > [local count: 1073741824]:
> >_1 = (int) c_3(D);
> >_2 = _1 << 22;
> >_4 = _2 != 0;
> >return _4;
> > }
> >
> > I wrote a patch to fix this problem in match.pd:
> >
> > +(match boolean_valued_p
> > + @0
> > + (if (TREE_CODE (type) == BOOLEAN_TYPE
> > + && TYPE_PRECISION (type) == 1)))
> > +(for op (tcc_comparison truth_and truth_andif truth_or truth_orif
> > truth_xor)
> > + (match boolean_valued_p
> > + (op @0 @1)))
> > +(match boolean_valued_p
> > + (truth_not @0))
> >
> > +/* cmp : ==, != */
> > +/* ((B0 << x) cmp 0) -> B0 cmp 0 */
> > +(for cmp (eq ne)
> > + (simplify
> > + (cmp (lshift (convert@3 boolean_valued_p@0) @1) integer_zerop@2)
> > + (if (TREE_CODE (TREE_TYPE (@3)) == INTEGER_TYPE
> > + && (GIMPLE || !TREE_SIDE_EFFECTS (@1)))
> > +(cmp @0 @2
> >
> >
> > But the problem is I am not able to restrict to the cases I am interested
> > in. There are many hits in other libraries I have tried compiling with
> > trunk+patch.
> >
> > Any feedback?
> >
> > 1)
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fbugzilla%2Fshow_bug.cgi%3Fid%3D98956&data=04%7C01%7Cnavidrahimi%40microsoft.com%7Caa8c9c8213a245c7ae9d08d9af9fc8ae%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637733923073627850%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=25KlLcsftTmN83rVawoKKaTPJdCdFlmtXMj%2BwsrKWbo%3D&reserved=0
> > 2)
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Falive2.llvm.org%2Fce%2Fz%2FUUTJ_v&data=04%7C01%7Cnavidrahimi%40microsoft.com%7Caa8c9c8213a245c7ae9d08d9af9fc8ae%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637733923073637846%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=fwN9%2BB0VObPyuUS2fOtj14i%2BHJIiRhyyjZM4LOF4AP8%3D&reserved=0
> It would help to also see the cases you're triggering that you do not
> want to trigger.
>
> Could we think of the optimization opportunity in a different way?
>
>
> (A << B) eq/ne 0 -> A eq/ne (0U >> B)
>
> And I would expect the 0U >> B to get simplified to 0.
>
> Would looking at things that way help?
>
> jeff
