Page MenuHomePhabricator

[Modules] Remove non-determinism while serializing DECL_CONTEXT_LEXICAL and DECL_RECORD
AbandonedPublic

Authored by bruno on Nov 26 2018, 6:01 PM.

Details

Summary

A module signature hashes out all relevant content in a module in order
to guarantee that in a dep chain, an inconsistent module never gets
imported. When using implicit modules, clang is capable of rebuilding a
module in case its signature doesn't match what's expected in the
importer. However, when using PCHs + implicit modules, clang cannot
rebuild a imported module with a signature mismatch, since it doesn't
know how to rebuild PCHs.

Non-determinism in the module content, can lead to changes to the
signature of a PCM across compiler invocations that are expected to
produce the same result, causing:

  • Build time penalties: rebuilding modules can be expensive.
  • Failures: when using PCHs, leads to hard errors one cannot recover

from.

This patch address non-determinism when serializing DECL_CONTEXT_LEXICAL
and DECL_RECORD by sorting the decls by ID (which is supposed to be
deterministic across multiple invocations) prior to writing out the
records. Another approach would be to fix all the places that trigger
adding a Decl to a DeclContext in non deterministic order, but I
couldn't spot any after an audit, but probably didn't look at everything
since the number of different sites and nested calls that do that is
pretty high. Also, by fixing it right before serialization, we enforce
that future changes also don't mess up with the order.

This isn't new, I consistently tracked this behavior in clang all the
way back to 2016, where my testcase stops working for other reasons.

I have no sensible and reduced testcases to add to this patch.

rdar://problem/43442957

Diff Detail

Event Timeline

bruno created this revision.Nov 26 2018, 6:01 PM

Although we can't synthesize a testcase for inclusion in the patch. Here is how one should attempt to reproduce the problem in macOS 10.14.

echo '@import Foundation;' > test.m
clang -fmodules test.m -o test.o -c \
      -fmodules-cache-path=/tmp/cache \
      -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
mv /tmp/cache /tmp/cache1

clang -fmodules test.m -o test.o -c \
      -fmodules-cache-path=/tmp/cache \
      -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
mv /tmp/cache /tmp/cache2

HASH=`ls /tmp/cache1`
for i in `find /tmp/cache1 -type f -iname "*.pcm"`; do
  F=`basename $i`;
  diff /tmp/cache1/$HASH/$F /tmp/cache2/$HASH/$F;
done
bruno added a comment.Nov 29 2018, 7:17 PM

@rsmith this is on the non-determinism issue we discussed offline. Do you have any concerns with this approach?

bruno abandoned this revision.Dec 13 2018, 2:48 PM

Abandon this one in favor of https://reviews.llvm.org/D55676