824817 - $OBJDIR/ipc/ipdl/PPluginInstanceChild.cpp:2837:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Daniel Holbert [:dholbert]]

I noticed a bunch of build warnings for $OBJDIR/ipc/PPlugin*.cpp of the form:
{
obj/ipc/ipdl/PPluginInstanceParent.cpp:2844:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized]
obj/ipc/ipdl/PPluginScriptableObjectParent.cpp:1522:27: warning: ‘tmp’ may be used uninitialized in this function [-Wuninitialized]
obj/ipc/ipdl/PPluginScriptableObjectChild.cpp:1511:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized]
obj/ipc/ipdl/PPluginInstanceChild.cpp:2837:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized]
}

In each case, it's in an impl of a method called "Read()".  Here's the code that the first warning above is for (and the others are for very-similar code):
> bool
> PPluginInstanceParent::Read(
>         SurfaceDescriptor* __v,
>         const Message* __msg,
>         void** __iter)
> {
>     switch (type) {
[...]
>     case __type::TPPluginSurfaceChild:
>         {
>             PPluginSurfaceParent* tmp;
>             (*(__v)) = tmp;
>             return Read((&((__v)->get_PPluginSurfaceParent())), __msg, __iter, false);
>         }

So we're declaring a pointer, "tmp" (initially with a random uninitialized address).  Then we copy that random, uninitialized address into *__v.  Then we do stuff with &__v.

I think this is probably innocuous -- from looking at contextual code, I suspect "do stuff with &__v" involves overwriting the random value.  (I think the "tmp" helper-var is intended for other cases, where we're dealing with a non-pointer type -- then, tmp is a freshly-initialized struct, and we can copy it into __v).

Still -- when we're dealing with a pointer type, the "tmp" usage is unnecessary & dangerous-looking. Can make our code generator smart enough to skip the "tmp" variable in this situation?

Comment 1

[Daniel Holbert [:dholbert]]

(Note: I hit this when building mozilla-inbound. Not sure if this is for code that's just been added there recently or for code that's been around for a while.)

Comment 2

[Daniel Holbert [:dholbert]]

(Actually, looks like this belongs in Core:Plug-ins, since the relevant source files live in /dom/plugins.)

Comment 3

[Daniel Holbert [:dholbert]]

I hit this error in a mozilla-central build today, too, so it's made it over there.

This does _not_ affect a 12-day-old mozilla-central build from this cset:
 https://hg.mozilla.org/mozilla-central/rev/50d8f411d305

In that build, the generated code that I quoted in comment 0 looks like this:
>    case __type::TPPluginSurfaceChild:
>         {
>             PPluginSurfaceParent* tmp = static_cast<PPluginSurfaceParent*>(0);
>             (*(__v)) = tmp;
>             return Read((&((__v)->get_PPluginSurfaceParent())), __msg, __iter, false);
>         }

...so -- we initialize |tmp| to null before reading its value, meaning it's not used uninitialized.

Comment 4

[Daniel Holbert [:dholbert]]

hg bisect points to this cset:
  https://hg.mozilla.org/mozilla-central/rev/fa47cd60942c
w/ commit message:
  Bug 819791 - Part 9: Use explicit TArray copy constructors in IPDL generated code. r=cjones

Comment 5

[Daniel Holbert [:dholbert]]

That commit's long-form commit message says:
> This cset makes two nop changes to generated IPDL code.
> 1) Change an instance of
> InfallibleTArray<Foo> foo = InfallibleTArray<Foo>();
> to
> InfallibleTArray<Foo> foo;
> 2) Change an instance of
> InfallibleTArray<Foo> foo = bar;
> to
> InfallibleTArray<Foo> foo(bar);

The sample-conversions there look similar to what changed here (comparing comment 3 to comment 0), except that in this bug's case, we've got raw-pointers, not TArrays.

It doesn't look like the commit was intended to convert raw-pointer "copy-constructor" usage, so this was almost certainly a side-effect rather than an intended result.

Comment 6

[Justin Lebar (not reading bugmail)]

> It doesn't look like the commit was intended to convert raw-pointer "copy-constructor" 
> usage, so this was almost certainly a side-effect rather than an intended result.

Yes, it was.  Sorry!

Comment 7

[Daniel Holbert [:dholbert]]

Ah, ok. Thanks for the clarification. :)

Is there any way we can check "Is this variable a pointer?" and skip the "declare a temp variable of the same type, initialize *__v with its contents" dance for pointers?

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: Eliminate warnings in IPC code.

These warnings are driving me batty. Proposed patch.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 700835 [details] [diff] [review]
Eliminate warnings in IPC code.

>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py

>+                decl = (StmtDecl(Decl(ct, tmpvar.name)) if not ct.ptr
>+                        else StmtDecl(Decl(ct, tmpvar.name), init=c.defaultValue()))
>                 readcase.addstmts([
>-                    StmtDecl(Decl(ct, tmpvar.name)),
>+                    decl,

Nit: clearer to my eyes is

                readcase.addstmts([
                    StmtDecl(Decl(ct, tmpvar.name,
                                  init=c.defaultValue() if ct.ptr else None)),

r=me with that.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: Eliminate warnings in IPC code.

Thanks Chris. Made the requested changes.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=34166d595a36

Comment 12

[Seth Fowler [:seth] [:s2h]]

Looks ok. Requesting checkin.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d879e26464a

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9d879e26464a