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

[Paolo Bonzini]

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