1024795 - Initialize the variables used to initialize IPDL unions where possible

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

btw, :seth fixed IPDL's related uninitialized pointer warnings in bug 824817.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove trailing whitespace from lower.py.

This was annoying me.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - static_cast isn't needed for nullptr in IPDL codegen.

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

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Generate nsTArray instead of InfallibleTArray in lower.py.

I think nsTArray is less odd than InfallibleTArray, which is just typedefed to it.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Use a default value for initializing unions, except for nsTArray.

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.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 9

[Andrew McCreight [:mccr8]]

(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.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

I think that sounds fine!

Comment 11

[Andrew McCreight [:mccr8]]

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 12

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Use a default value for initializing unions, except for nsTArray.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 14

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb0d8d53ef9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9afbb34645
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7945e7a05f8f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/642e73d314c3

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9bb0d8d53ef9
https://hg.mozilla.org/mozilla-central/rev/ae9afbb34645
https://hg.mozilla.org/mozilla-central/rev/7945e7a05f8f
https://hg.mozilla.org/mozilla-central/rev/642e73d314c3