From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQiXE-0007tb-1A for qemu-devel@nongnu.org; Thu, 20 Mar 2014 15:21:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQiX7-0003Bq-Dx for qemu-devel@nongnu.org; Thu, 20 Mar 2014 15:21:47 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:55653) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQiX7-0003BX-7a for qemu-devel@nongnu.org; Thu, 20 Mar 2014 15:21:41 -0400 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 20 Mar 2014 13:21:40 -0600 Received: from b01cxnp23033.gho.pok.ibm.com (b01cxnp23033.gho.pok.ibm.com [9.57.198.28]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id C5CF338C804F for ; Thu, 20 Mar 2014 15:21:36 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by b01cxnp23033.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2KJLacu9437520 for ; Thu, 20 Mar 2014 19:21:36 GMT Received: from d01av04.pok.ibm.com (localhost [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2KJLaXC029276 for ; Thu, 20 Mar 2014 15:21:36 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <87bnx3vomf.fsf@blackfin.pond.sub.org> References: <87bnx3vomf.fsf@blackfin.pond.sub.org> Message-ID: <20140320192134.8983.86526@loki> Date: Thu, 20 Mar 2014 14:21:34 -0500 Subject: Re: [Qemu-devel] qapi-commands.py generates code that uses uninitialized variables List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Peter Maydell Cc: QEMU Developers , Anthony Liguori Quoting Markus Armbruster (2014-03-18 04:32:08) > Peter Maydell writes: > = > > This is something clang's -fsanitize=3Dundefined spotted. The > > code generated by qapi-commands.py in qmp-marshal.c for > > qmp_marshal_* functions where there are some optional > > arguments looks like this: > > > > bool has_force =3D false; > > bool force; > > > > mi =3D qmp_input_visitor_new_strict(QOBJECT(args)); > > v =3D qmp_input_get_visitor(mi); > > visit_type_str(v, &device, "device", errp); > > visit_start_optional(v, &has_force, "force", errp); > > if (has_force) { > > visit_type_bool(v, &force, "force", errp); > > } > > visit_end_optional(v, errp); > > qmp_input_visitor_cleanup(mi); > > > > if (error_is_set(errp)) { > > goto out; > > } > > qmp_eject(device, has_force, force, errp); > > > > In the case where has_force is false, we never initialize > > force, but then we use it by passing it to qmp_eject. > > I imagine we don't then actually use the value, but clang > = > Use of FOO when !has_FOO is a bug. > = > > complains in particular for 'bool' variables because the value > > that ends up being loaded from memory for 'force' is not either > > 0 or 1 (being uninitialized stack contents). > > > > Anybody understand what the codegenerator is doing well enough > > to suggest a fix? I'd guess that just initializing the variable either > > at point of declaration or in an else {) clause of the 'if (has_force)' > > conditional would suffice, but presumably you need to handle > > all the possible data types... > = > I can give it a try. Will probably take a while, though. Could it be as simple as this?: diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 9734ab0..a70482e 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -99,7 +99,7 @@ bool has_%(argname)s =3D false; argname=3Dc_var(argname), argtype=3Dc_type(argtyp= e)) else: ret +=3D mcgen(''' -%(argtype)s %(argname)s; +%(argtype)s %(argname)s =3D {0}; ''', argname=3Dc_var(argname), argtype=3Dc_type(argtyp= e)) Pointer-type are special-cased initialized to NULL, so that leaves these gu= ys in the current set of qapi-defined types that we use as direct arguments for qmp commands: NON-POINTER TYPE: BlockdevOnError NON-POINTER TYPE: bool NON-POINTER TYPE: DataFormat NON-POINTER TYPE: double NON-POINTER TYPE: DumpGuestMemoryFormat NON-POINTER TYPE: int64_t NON-POINTER TYPE: MirrorSyncMode NON-POINTER TYPE: NewImageMode NON-POINTER TYPE: uint32_t I'm trying to make sense of whether {0} is a valid initializer in all these cases, as I saw some references to GCC complaining about cases where you do= n't use an initializer for each nested subtype (back in 2002 at least: http://www.ex-parrot.com/~chris/random/initialise.html), but that doesn't s= eem to be the case now. If that's not safe, we can memset based on sizeof() in the else clause, but obviously that's sub-optimal.