Closed Bug 824817 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dholbert, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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?
(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.)
Summary: /obj/ipc/ipdl/PPluginInstanceChild.cpp:2837:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized] → $OBJDIR/ipc/ipdl/PPluginInstanceChild.cpp:2837:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized]
(Actually, looks like this belongs in Core:Plug-ins, since the relevant source files live in /dom/plugins.)
Component: IPC → Plug-ins
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.
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
Blocks: 819791
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.
> 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!
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?
Component: Plug-ins → IPC
Attached patch Eliminate warnings in IPC code. (obsolete) — Splinter Review
These warnings are driving me batty. Proposed patch.
Attachment #700835 - Flags: review?(jones.chris.g)
Assignee: nobody → seth
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.
Attachment #700835 - Flags: review?(jones.chris.g) → review+
Thanks Chris. Made the requested changes.
Attachment #700835 - Attachment is obsolete: true
Looks ok. Requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9d879e26464a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.