Patchwork [C++] Fix PR 55418

login
register
mail settings
Submitter Markus Trippelsdorf
Date Nov. 22, 2012, 12:06 p.m.
Message ID <20121122120654.GA217@x4>
Download mbox | patch
Permalink /patch/201038/
State New
Headers show

Comments

Markus Trippelsdorf - Nov. 22, 2012, 12:06 p.m.
Hi,

Valgrind complains about an uninitialized variable in method.c
(implicitly_declare_fn):  

==19553== Conditional jump or move depends on uninitialised value(s)
==19553==    at 0x63D248: implicitly_declare_fn(special_function_kind, tree_node*, bool, tree_node*, tree_node*) (method.c:1623)
==19553==    by 0x63EA8F: lazily_declare_fn(special_function_kind, tree_node*) (method.c:1894)
==19553==    by 0x6455EC: lookup_fnfields_1(tree_node*, tree_node*) (search.c:1441)
==19553==    by 0x64577B: lookup_fnfields_slot(tree_node*, tree_node*) (search.c:1471)
==19553==    by 0x64931B: lookup_field_r(tree_node*, void*) (search.c:1031)
==19553==    by 0x645B08: dfs_walk_all(tree_node*, tree_node* (*)(tree_node*, void*), tree_node* (*)(tree_node*, void*), void*) (search.c:1572)
==19553==    by 0x645D5D: lookup_member(tree_node*, tree_node*, int, bool, int) (search.c:1204)
==19553==    by 0x646010: lookup_fnfields(tree_node*, tree_node*, int) (search.c:1295)
==19553==    by 0x4E6E50: build_special_member_call(tree_node*, tree_node*, vec<tree_node*, va_gc, vl_embed>**, tree_node*, int, int) (call.c:7282)
==19553==    by 0x4E0DEA: convert_like_real(conversion*, tree_node*, tree_node*, int, int, bool, bool, int) (call.c:5718)
==19553==    by 0x4E28D0: build_over_call(z_candidate*, int, int) (call.c:6867)
==19553==    by 0x4E5C28: build_new_method_call(tree_node*, tree_node*, vec<tree_node*, va_gc, vl_embed>**, tree_node*, int, tree_node**, int) (call.c:7668)
==19553==

The fix is trivial. I would appreciate if someone could commit this.
Thanks.

Bootstrapped and tested on x86_64-pc-linux-gnu.

2012-11-22  Markus Trippelsdorf  <markus@trippelsdorf.de>

	PR c++/55418
	* method.c (implicitly_declare_fn): Properly initialize trivial_p.
Paolo Carlini - Nov. 22, 2012, 1:28 p.m.
Hi,

On 11/22/2012 01:06 PM, Markus Trippelsdorf wrote:
> The fix is trivial. I would appreciate if someone could commit this.
I agree it qualifies as obvious and I mean to commit the patch on your 
behalf before the end of the day if nobody objects. To be super-safe, I 
guess I will also run the testsuite with the current code and trivial_p 
initialized to true at the beginning of the function.

Thanks!
Paolo.
Jakub Jelinek - Nov. 22, 2012, 3:06 p.m.
On Thu, Nov 22, 2012 at 02:28:18PM +0100, Paolo Carlini wrote:
> On 11/22/2012 01:06 PM, Markus Trippelsdorf wrote:
> >The fix is trivial. I would appreciate if someone could commit this.
> I agree it qualifies as obvious and I mean to commit the patch on
> your behalf before the end of the day if nobody objects. To be
> super-safe, I guess I will also run the testsuite with the current
> code and trivial_p initialized to true at the beginning of the
> function.

I think trivial_p used to be uninitialized only for deleted_p functions,
not sure if those are considered trivial or not (and whether it matters)...

	Jakub
Paolo Carlini - Nov. 22, 2012, 4:03 p.m.
Hi,

On 11/22/2012 04:06 PM, Jakub Jelinek wrote:
> On Thu, Nov 22, 2012 at 02:28:18PM +0100, Paolo Carlini wrote:
>> On 11/22/2012 01:06 PM, Markus Trippelsdorf wrote:
>>> The fix is trivial. I would appreciate if someone could commit this.
>> I agree it qualifies as obvious and I mean to commit the patch on
>> your behalf before the end of the day if nobody objects. To be
>> super-safe, I guess I will also run the testsuite with the current
>> code and trivial_p initialized to true at the beginning of the
>> function.
> I think trivial_p used to be uninitialized only for deleted_p functions,
> not sure if those are considered trivial or not (and whether it matters)...
I think that the Valgrind complain is benign and, by the way, I just run 
the testsuite with current code + trivial_p initialized to true with no 
regressions. In detail what is happening is that, as you say, in the 
special case of deleted_p true and lambdas, we return from 
synthesized_method_walk and trivial_p is still uninitialized. Then we have:

   if (!trivial_p && type_has_trivial_fn (type, kind))
     type_set_nontrivial_flag (type, kind);

which, if trivial_p is is still uninitialized may not do its job of 
forcing nontrivial set in type: I don't think this is a serious problem 
for lambdas (in any case, when code elsewhere detects deleted shouldn't 
bother anyway of checking triviality; testsuite passes with trivial_p 
initialized to true too), but Markus (and me ;) proposes anyway to 
initialize trivial_p to false, thus we should be fine (way below 
trivial_p is used again but only when deleted_p is false).

Paolo.
Jason Merrill - Nov. 23, 2012, 3:46 p.m.
On 11/22/2012 11:03 AM, Paolo Carlini wrote:
> initialized to true too), but Markus (and me ;) proposes anyway to
> initialize trivial_p to false, thus we should be fine (way below
> trivial_p is used again but only when deleted_p is false).

It certainly can't hurt.

Jason
Jason Merrill - Nov. 23, 2012, 3:47 p.m.
On 11/23/2012 10:46 AM, Jason Merrill wrote:
> On 11/22/2012 11:03 AM, Paolo Carlini wrote:
>> initialized to true too), but Markus (and me ;) proposes anyway to
>> initialize trivial_p to false, thus we should be fine (way below
>> trivial_p is used again but only when deleted_p is false).
>
> It certainly can't hurt.

Actually...we should be calculating triviality normally regardless of 
whether the function is deleted.  Why isn't that happening?

Jason
Paolo Carlini - Nov. 23, 2012, 3:50 p.m.
Hi,

On 11/23/2012 04:47 PM, Jason Merrill wrote:
> On 11/23/2012 10:46 AM, Jason Merrill wrote:
>> On 11/22/2012 11:03 AM, Paolo Carlini wrote:
>>> initialized to true too), but Markus (and me ;) proposes anyway to
>>> initialize trivial_p to false, thus we should be fine (way below
>>> trivial_p is used again but only when deleted_p is false).
>>
>> It certainly can't hurt.
>
> Actually...we should be calculating triviality normally regardless of 
> whether the function is deleted.  Why isn't that happening?
Because in synthesized_method_walk for lambdas we have this early return:

   if (deleted_p)
     {
       /* "The closure type associated with a lambda-expression has a 
deleted
      default constructor and a deleted copy assignment operator."
          This is diagnosed in maybe_explain_implicit_delete.  */
       if (LAMBDA_TYPE_P (ctype)
       && (sfk == sfk_constructor
           || sfk == sfk_copy_assignment))
     {
       *deleted_p = true;
       return;
     }

       *deleted_p = false;
     }

Paolo.

Patch

diff --git a/gcc/cp/method.c b/gcc/cp/method.c
index 6dcb63a..778daa8 100644
--- a/gcc/cp/method.c
+++ b/gcc/cp/method.c
@@ -1518,7 +1518,6 @@  implicitly_declare_fn (special_function_kind kind, tree type,
   tree name;
   HOST_WIDE_INT saved_processing_template_decl;
   bool deleted_p;
-  bool trivial_p;
   bool constexpr_p;
 
   /* Because we create declarations for implicitly declared functions
@@ -1597,12 +1596,13 @@  implicitly_declare_fn (special_function_kind kind, tree type,
   tree inherited_base = (inherited_ctor
 			 ? DECL_CONTEXT (inherited_ctor)
 			 : NULL_TREE);
+  bool trivial_p = false;
+
   if (inherited_ctor && TREE_CODE (inherited_ctor) == TEMPLATE_DECL)
     {
       /* For an inheriting constructor template, just copy these flags from
 	 the inherited constructor template for now.  */
       raises = TYPE_RAISES_EXCEPTIONS (TREE_TYPE (inherited_ctor));
-      trivial_p = false;
       deleted_p = DECL_DELETED_FN (DECL_TEMPLATE_RESULT (inherited_ctor));
       constexpr_p
 	= DECL_DECLARED_CONSTEXPR_P (DECL_TEMPLATE_RESULT (inherited_ctor));