[MetadataReader] Initialize fields to prevent UB.

[dcci]

hasVtable is not initialized on all paths, so when we read it,
we end up reading a random value. On debug builds, this value
almost always happen to be zero, so we do the right thing. This
isn't necessarily true when the optimizations kick in.

lldb uses metadata reder to resolve types and as result of this
undefined behaviour sometimes gets wrong results back, causing
a bunch of tests to fail in release builds.

[dcci]

@swift-ci please smoke test and merge

[jckarter]

Doh, good catch!

[jckarter]

We should get warnings from Clang if we miss an initialization in the switch below. (Not sure why we didn't see one for hasVTable…). Instead of pre-initializing the values here, does it work to initialize hasVTable = false; in the Module, Extension, and Anonymous branches? It looks like baseSize already gets initialized on every path, and if we add new cases in the future, it'd be nice to get the compiler warnings to ensure they're set intentionally for the new case.

[dcci]

yes, it works, but I'm afraid this will show up again if somebody adds a new case to the switch.
Maybe an alternative would be having a three state boolean (and then add an assertion at the end of the switch/case to make sure it's initialized?

FWIW, this PR/problem is one of the reasons why I think languages without definite initialization semantics for types are particularly hard to work with).

[jckarter]

Yeah, that's the weird thing—Clang does have definite initialization warnings for trivial types that should have fired here.

[dcci]

Also, fun fact, it seems that some tests in swift/test happen to fail under ubsan with your commit, but the issue was hidden by other failures early in the build (the UBsan build is not clean). I think we can try to put some energy to get the UBsan build green for swift as a second line of defense (although, for simple types like bool or int, clang warnings should always fire)

[dcci]

oh, sigh, clang actually warns on this only if -Wuninitialized is passed (which is part of -Wall but not of the default set of warnings).

see, e.g.

int patatino(int banana) {
  int tinkywinky = 27;
  bool initializeMe;

  switch (banana) {
    case 0:
      break;
    case 1:
      break;
    case 2:
      //initializeMe = false;
      break;
    default:
      break;
  }

  if (initializeMe) {
    tinkywinky = 12;
  }

  return tinkywinky;
}

dcci@Davides-MacBook-Pro ~> clang++ patatino.cpp -c
dcci@Davides-MacBook-Pro ~> clang++ patatino.cpp -c -Wall
patatino.cpp:17:7: warning: variable 'initializeMe' is uninitialized when used
      here [-Wuninitialized]
  if (initializeMe) {
      ^~~~~~~~~~~~
patatino.cpp:3:20: note: initialize the variable 'initializeMe' to silence this
      warning
  bool initializeMe;
                   ^
                    = false
1 warning generated.

Should we consider enabling -Wuninitialized (or -Wall) for the swift codebase? IIRC the reason why warnings are part of -Wall but not of the default set is because they might produce false positive, but you could always annotate places to placate them.

[jckarter]

Hm, I think we have -Wuninitialized on for the compiler, but maybe not the runtime. I think we ought to have it on for both.

[dcci]

@jckarter I'll be more than happy to submit a pull request to enable this tomorrow (and fix the failures, if any).

[jckarter]

Sounds great! Thanks again for tracking this down and fixing it.

[AnnaZaks]

Can we enable not only -Wuninitialized but -Wall for the runtime?

[dcci]

@AnnaZaks yes, it's reasonable, let me do it now.

[AnnaZaks]

Also, is it possible to add a test case in the swift codebase that would exercise the code or does it already exist?

[jckarter]

There are already tests for this code in the Swift codebase. Somehow they all managed to get lucky and not trip on the uninitialized variable here.