Closed
Bug 824817
Opened 8 years ago
Closed 8 years ago
$OBJDIR/ipc/ipdl/PPluginInstanceChild.cpp:2837:27: warning: 'tmp' may be used uninitialized in this function [-Wuninitialized]
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: dholbert, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
1.23 KB,
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•8 years ago
|
||
(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]
Reporter | ||
Comment 2•8 years ago
|
||
(Actually, looks like this belongs in Core:Plug-ins, since the relevant source files live in /dom/plugins.)
Component: IPC → Plug-ins
Reporter | ||
Updated•8 years ago
|
Blocks: buildwarning
Reporter | ||
Comment 3•8 years ago
|
||
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.
Reporter | ||
Comment 4•8 years ago
|
||
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
Reporter | ||
Comment 5•8 years ago
|
||
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•8 years ago
|
||
> 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!
Reporter | ||
Comment 7•8 years ago
|
||
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?
Updated•8 years ago
|
Component: Plug-ins → IPC
Assignee | ||
Comment 8•8 years ago
|
||
These warnings are driving me batty. Proposed patch.
Attachment #700835 -
Flags: review?(jones.chris.g)
Assignee | ||
Updated•8 years ago
|
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+
Assignee | ||
Comment 10•8 years ago
|
||
Thanks Chris. Made the requested changes.
Assignee | ||
Updated•8 years ago
|
Attachment #700835 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=34166d595a36
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d879e26464a
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9d879e26464a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•