Init index_in_metadata_ without condition

[jbrianceau]

Chromium MemorySanitizer (MSan) reports a use-of-uninitialized-value
warning because of index_in_metadata_ attribute of EnumGenerator
class (introduced in 5a76e63). Fix
it by initializing it to 0.

[grpc-kokoro]

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

[grpc-kokoro]

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

[bazel-io]

Can one of the admins verify this patch?

[jbrianceau]

See https://codereview.chromium.org/2590803003/ for further details.
@acozzette, @xfxyjwf for review.

[acozzette]

ok to test

[acozzette]

@jbrianceau Thanks for sending this pull request. It seems to me like the root of the problem is that for each kind of generator, we assign that index_in_metadata_ variable here, but that's inside a block that is only executed if HasDescriptorMethods(...) is true (i.e. only for non-lite). So I think the right solution would be to change that code so that we assign the variable unconditionally, regardless of whether we're using lite. I think with your current solution the variable is always assigned 0 in lite mode, and this works for now only because we never actually use it for lite. But if we end up needing it for lite in the future it will be confusing because we will get the wrong value (always 0).

Sorry about the test failures by the way; I believe those were caused by a separate thing I broke and should now be fixed.

[acozzette]

retest this please

[jbrianceau]

So I think the right solution would be to change that code so that we assign the variable unconditionally, regardless of whether we're using lite.

You're right. If I add a else case for HasDescriptorMethods(...) looking like this, it fixes MSan warning too :

  if (HasDescriptorMethods(file_, options_)) {
    [...]
  } else {
    for (int i = 0; i < enum_generators_.size(); i++) {
      // Set index_in_metadata_ value to avoid use-of-uninitialized-value warning
      enum_generators_[i]->index_in_metadata_ = i;
    }
  }

But if we end up needing it for lite in the future it will be confusing because we will get the wrong value (always 0).

Setting it to zero in EnumGenerator constructor shouldn't prevent it to be set to the right value after, am I missing something ?

What solution do you prefer @acozzette ? Current patch + the above else case or just the above else case (i.e. no init in EnumGenerator's constructor)

[acozzette]

That's true, setting it to zero wouldn't prevent it from being set to the right value later on. I guess I would lean slightly toward not setting it in the constructor, though; that way if we forget to set it again then MSAN will hopefully catch the bug like it did this time.

About the else case, maybe instead of adding an else you could just move the code outside of the if block so that it's not duplicated. I think we probably want to do this for message_generators_ and service_generators_ in addition to enum_generators.

I guess the message_generators_ one is a little complicated because part of that probably has to happen inside the if since it has a side effect (from the GenerateDescriptorDeclarations call):

for (int i = 0; i < message_generators_.size(); i++) {
  message_generators_[i]->index_in_metadata_ = i;
  message_generators_[i]->GenerateDescriptorDeclarations(printer);
}

So I would say maybe have three loops for initializing index_in_metadata_ before the if block, and but keep this inside the if:

for (int i = 0; i < message_generators_.size(); i++) {
  message_generators_[i]->GenerateDescriptorDeclarations(printer);
}

Does that work?

[jbrianceau]

that way if we forget to set it again then MSAN will hopefully catch the bug like it did this time.

Good point, agreed.

Does that work?

Yes, I've just tested it quickly in my Chromium build and it seems to be ok.
I'm going to update the PR.

[jbrianceau]

@acozzette, please take another look

[acozzette]

Thanks, @jbrianceau!