Re: use of uninitialized variable involving visit_type_uint32() and friends

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 4/1/22 15:11, Markus Armbruster wrote:
>>> If it can do really serious interprocedural analysis, it _might_ be able
>>> to see through the visitor constructor and know that the "value = *obj"
>>> is not initialized (e.g. "all callers of object_property_set use an
>>> input visitor").  I doubt that honestly, but a man can dream.
>> 
>> I'm wary of arguments based on "a sufficiently smart compiler can"...
>
> Absolutely.
>
>>> Because it communicates what the caller expects: "I have left this
>>> uninitialized because I expect my "v" argument to be the kind of visitor
>>> that fills it in".  It's this argument that gives me the confidence
>>> needed to shut up Coverity's false positives.
>>>
>>> Embedding the visitor type in the signature makes it impossible not to
>>> pass it, unlike e.g. an assertion in every getter or setter.
>> 
>> I think we got two kinds of code calling visitor methods:
>> 
>> 1. Code for use with one kind of visitor only
>> 
>>     We get to pass a literal argument to the additional parameter you
>>     propose.
>> 
>> 2. Code for use with arbitrary visitors (such as qapi-visit*.c)
>> 
>>     We need to pass v->type, where @v is the existing visitor argument.
>>     Except we can't: struct Visitor and VisitorType are private, defined
>>     in <visitor-impl.h>.  Easy enough to work around, but has a distinct
>>     "this design is falling apart" smell, at least to me.
>
> Hmm, maybe that's a feature though.  If we only need v->type in .c files 
> for the generated visit_type_* functions, then it's not a huge deal that 
> they will have to include <visitor-impl.h>.  All callers outside 
> generated type visitors (which includes for example QMP command 
> marshaling), instead, would _have_ to pass visitor type constants and 
> make it clear what direction the visit is going.

I quoted the generated qapi-visit*.c as an example.  There may
handwritten instances, too.

>> Note that "intent explicit in every method call" is sufficient, but not
>> necessary for "intent is locally explicit, which lets us dismiss false
>> positives with confidence".  We could do "every function that calls
>> methods".  Like checking a precondition.  We already have
>> visit_is_input().  We could have visit_is_output().
>> 
>> The sane way to make output intent explicit is of course passing the
>> thing by value rather than by reference.  To get that, we could generate
>> even more code.  So, if the amount of code we currently generate isn't
>> disgusting enough, ...
>
> Yeah, that would be ugly.  Or, we could generate the same code plus some 
> static inline wrappers that take a
>
>    struct InputVisitor {
>        Visitor dont_use_me_it_hurts;
>    }
>    struct OutputVisitor {
>        Visitor dont_use_me_it_hurts;
>    }
>
> That would be zero-cost abstraction at runtime.

Looks worth exploring!