Page MenuHomePhabricator

[Modules] Fix decl order for DeclsInPrototype
Needs ReviewPublic

Authored by bruno on Dec 13 2018, 2:48 PM.

Details

Summary

When a declarator is constructed for a function prototype in
Parser::ParseFunctionDeclarator, it calls getCurScope()->decls()
in order to populate DeclsInPrototype.

getCurScope()->decls() return a range from a llvm::SmallPtrSet, which
doesn't guarantee any order. Later on, DeclsInPrototype are used to
populate decl contexts that end up being serialized when PCH or modules
are used. This causes non-determinism in module serialization, which
is bad for lots of reason, including that when a PCH used Modules, it
can't rebuild the module if there's a signature mismatch.

This patch fixes that by sorting out the result getCurScope()->decls()
before populating DeclsInPrototype. I don't believe we can change this
data structure because it can be used for any type of scope (not only
function prototypes).

There are no testcases for this change, since you have to run the
problematic non-reducible testcase multiple times to get a mismatch.
However, one can try to reproduce it on macOS using:

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

rdar://problem/43442957

Diff Detail

Event Timeline

bruno created this revision.Dec 13 2018, 2:48 PM
bruno added a comment.Dec 13 2018, 2:49 PM

This fixes the same problem previously addressed in https://reviews.llvm.org/D54923

rsmith added inline comments.Jan 22 2019, 11:22 AM
lib/Parse/ParseDecl.cpp
6237

I would prefer that we use getID only for debug dumping purposes. Can we change Scope::DeclsInContext to be a SetVector instead? (How much does that cost us memory-wise?)