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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

> 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.

Paolo