Initialize the variables used to initialize IPDL unions where possible

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 2 bugs, {coverity})

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

There's a pattern that the generated IPDL code does that gives Coverity fits, and it looks kind of gross.

uint64_t_ tmp;
(*(__v)) = tmp;
...read into v...

So we have this totally uninitialized tmp value, which we then assign into __v.  I think what this does is it sets some kind of flag on v to indicate that it is a uint64 holding thing, thanks to some overload, then copies the garbage tmp into v.  Now, this is okay because we then overwrite the garbage data we copied into, but it is still kind of gross.

This could be changed by either just initializing tmp, or by changing this weird implicit overload pattern to some kind of, say, templated thing that sets the flag on v without copying garbage.
btw, :seth fixed IPDL's related uninitialized pointer warnings in bug 824817.
Depends on: 824817
I think I tried making this work at one point by not always initializing __v, but by setting some sort of flag (because this pattern is inefficient, too), and IPC tests started going bonkers.
Assignee: nobody → continuation
I'm thinking the best solution here may be to restore the default constructor for everything except for whatever nsTArray stuff that can't handle it.
This was annoying me.
Attachment #8468007 - Flags: review?(bent.mozilla)
Since bug 1038523, lower.py generates nullptr for NULL, so this cast isn't needed.

try run: https://tbpl.mozilla.org/?tree=Try&rev=5308d974031b
Attachment #8468008 - Flags: review?(bent.mozilla)
I think nsTArray is less odd than InfallibleTArray, which is just typedefed to it.
Attachment #8468009 - Flags: review?(bent.mozilla)
This restores the initialization behavior prior to bug 819791, except for the one case that bug requires.

This is a little hacky.  I could add some kind of hasimplicitcopyctor method to Type, and have it return true for everything but nsTArray, but that feels like overkill for this one particular case.
Attachment #8468010 - Flags: review?(bent.mozilla)
Summary: It would be nice if IPDL set the type of scalar GetterSetters without copying garbage → Initialize the variables used to initialize IPDL unions where possible
Attachment #8468007 - Flags: review?(bent.mozilla) → review+
Attachment #8468008 - Flags: review?(bent.mozilla) → review+
Attachment #8468009 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8468010 [details] [diff] [review]
part 4 - Use a default value for initializing unions, except for nsTArray.

Review of attachment 8468010 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/lower.py
@@ +850,5 @@
>  
>      def defaultValue(self):
> +        # nsTArray does not have an implicit copy constructor, so use the
> +        # default constructor on the declaration instead.
> +        if self.bareType().name == 'nsTArray':

Is there a way to do this check in terms of |_cxxArrayType|?

@@ +4696,5 @@
>                      c = c.other       # see above
>                  tmpvar = ExprVar('tmp')
>                  ct = c.bareType()
>                  readcase.addstmts([
> +                    StmtDecl(Decl(ct, tmpvar.name), init=c.defaultValue()),

Hrm, I'd love to see a diff of the code this generates.
Attachment #8468010 - Flags: review?(bent.mozilla)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #8)
> Comment on attachment 8468010 [details] [diff] [review]
> part 4 - Use a default value for initializing unions, except for nsTArray.
> 
> Review of attachment 8468010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/lower.py
> @@ +850,5 @@
> >  
> >      def defaultValue(self):
> > +        # nsTArray does not have an implicit copy constructor, so use the
> > +        # default constructor on the declaration instead.
> > +        if self.bareType().name == 'nsTArray':
> 
> Is there a way to do this check in terms of |_cxxArrayType|?

I could define a new string for 'nsTArray' in the file, and then use it both in _cxxArrayType and defaultValue().  Alternatively, I could add a new property like hasimplicitcopyconstructor on Type, and set it to False for _cxxArrayType, then check that property in defaultValue().  That would be less hacky, but perhaps overkill.

> Hrm, I'd love to see a diff of the code this generates.

Sure, I can do that.
(In reply to Andrew McCreight [:mccr8] from comment #9)
> That would be less hacky, but perhaps overkill.

I think that sounds fine!
Here are some examples of the changes:
<             MIMEInputStreamParams tmp;
>             MIMEInputStreamParams tmp = MIMEInputStreamParams();

<             void_t tmp;
>             void_t tmp = void_t();

<             uint64_t tmp;
>             uint64_t tmp = uint64_t();

This is the same as the behavior prior to bug 819791.

Things like this:
  PHttpChannelChild* tmp = nullptr;
  nsTArray<nsString> tmp;
...stay the same.
Comment on attachment 8470249 [details] [diff] [review]
part 4 - Use a default value for initializing unions, except for nsTArray.

Review of attachment 8470249 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8470249 - Flags: review?(bent.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.