Page MenuHomePhabricator

Fix PR32332 - PCM nondeterminism with builtins caused by global module index

Authored by aprantl on Jul 26 2017, 3:54 PM.



Long story short: I think that it is too dangerous to rely on the global module index for anything but diagnostics.

The global module index is only rebuilt in two cases:

a. When triggered by Sema for diagnosing errors
b. At the very end of FrontendAction::Execute()

This patch is deleting a shortcut in ModuleManager::visit() that uses the global module index as a negative cache to determine that it does not make sense to open a module when looking for a specific identifier.

In the test case (see also the PR we get different PCM output for B.pcm when

  1. building B.pcm (which also builds the dependency A.pcm in one go)
  2. building A.pcm, then building B.pcm in a separate clang invocation

Because of when the global module index is built, (1) does not use the above shortcut, but (2) does. The symbol were this makes a difference is a built-in macro FLT_EPSILON which does not belong to any module but still makes it into the global module index as not being defined in any module. This causes the symbol to be serialized in (1) A.pcm and (2) A.pcm and B.pcm.

I have considered the alternative of hunting down why builtins are serialized into the PCM in the first place, but frankly I feel like this is just the tip of the iceberg, and that relying on the global module index this way is just asking for trouble in concurrent or incremental builds. I have looked at the performance impact of the change, and in the project I built (a large project, ~30min build time) the wall-clock numbers were in the noise.


Diff Detail

Event Timeline

aprantl created this revision.Jul 26 2017, 3:54 PM
doug.gregor edited edge metadata.Jul 26 2017, 4:56 PM

I am *very* concerned about this. The optimization that avoids doing lookups within module files where we "know" we won't find anything was very important at the time it was introduced, so I'd like to understand better why it didn't make a significant difference in your benchmarks before we disable it. (Especially because of built-ins; those are Very Weird and probably modeled wrong).

After thinking about this some more, I think it should be safe to use the global module index as a negative cache like in this example. The conditions under which a module is rebuilt should not affect the contents, and if the module were out-of-date it would have been rebuilt anyway.
I will see if I can track down why the builtin macros make it into the module's identifier table. We already know that they are being marked incorrectly as FromAST, but I do not yet understand why.

aprantl abandoned this revision.Aug 31 2017, 10:51 AM