On 20 March 2014 19:21, Michael Roth <mdr...@linux.vnet.ibm.com> wrote:
> Quoting Markus Armbruster (2014-03-18 04:32:08)
>> Peter Maydell <peter.mayd...@linaro.org> writes:
>>
>> > This is something clang's -fsanitize=undefined 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 = false;
>> > bool force;
>> >
>> > mi = qmp_input_visitor_new_strict(QOBJECT(args));
>> > v = 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 = false;
> argname=c_var(argname), argtype=c_type(argtype))
> else:
> ret += mcgen('''
> -%(argtype)s %(argname)s;
> +%(argtype)s %(argname)s = {0};
> ''',
> argname=c_var(argname), argtype=c_type(argtype))
>
What's the status of this? I noticed that clang is now
complaining at compile time as well as runtime:
CC tests/test-qmp-marshal.o
tests/test-qmp-marshal.c:181:9: warning: variable 'b' is used
uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]
if (has_b) {
^~~~~
tests/test-qmp-marshal.c:188:42: note: uninitialized use occurs here
retval = qmp_user_def_cmd3(a, has_b, b, &local_err);
^
tests/test-qmp-marshal.c:181:5: note: remove the 'if' if its condition
is always true
if (has_b) {
^~~~~~~~~~~
tests/test-qmp-marshal.c:170:14: note: initialize the variable 'b' to
silence this warning
int64_t b;
^
= 0
1 warning generated.
thanks
-- PMM
