Modular Codegen: Cross-module deserialization problem

dblaikie · February 24, 2017, 5:41pm

I’ve spent a few days debugging/trying to understand the following, so I’m writing it all down in the hopes of getting it straight/clear and potentially getting some external perspective on what’s going on, whether I’ve understood it correctly, and what might be a good way to solve it.

Starting with this test case:
foo.h:

struct foo {};
inline void e() { foo(); }
bar.h:

#include “foo.h”
template
foo bar(foo &f) {
return f;
}
void z() { (void)&bar; }

Build each into a separate module, run modular codegen on bar.pcm:

clang-tot -cc1 -fmodules-codegen -xc++ -emit-module -fmodules
-fmodule-name=foo foo.cppmap -o foo.pcm
clang-tot -cc1 -fmodules-codegen -xc++ -emit-module -fmodules
-fmodule-name=bar bar.cppmap -o bar.pcm -fmodule-file=foo.pcm
clang-tot -c bar.pcm -o bar.o

(I haven’t fully understood why the use of ‘foo’ in foo.h is necessary, nor why ‘bar’ needs to be a template - those might provide some further insight about how this should/could work)

This patch makes the failure a bit more visible/immediate:

diff --git lib/Serialization/ASTReaderStmt.cpp lib/Serialization/ASTReaderStmt.cpp
index b4718367d4…1fb48e9560 100644
@@ -3903,7 +3904,9 @@ Stmt *ASTReader::ReadStmtFromStream(ModuleFile &F) {
++NumStatementsRead;

if (S && !IsStmtReference) {

auto X = Cursor.GetCurrentBitNo();
Reader.Visit(S);
assert(X == Cursor.GetCurrentBitNo() && “Narf”);
StmtEntries[Cursor.GetCurrentBitNo()] = S;
}

This is layered on top of the modular codegen rewrite/refactor (to use a bit on function definitions, instead of to imply modular codegen from the Module object) and some other fixes. I’ll include the full patch I’m working with.

The sequence of relevant steps (as best as I can figure)

bar() body deserialization begins
bar module’s DeclCursor is used, jumping to the start of the bar()'s definition
clang::ASTReader::ReadStmtFromStream iterates through Stmts in bar()
no SavedStreamPosition nor Deserializing object here
deserializing the EXPR_CXX_CONSTRUCT gets interesting:
eventually involves deserializing foo(const foo&)
foo module’s DeclCursor is used
foo module’s DeclCursor is saved/preserved with a SavedStreamPosition
a ExternalASTSource::Deserializing context is started
the definition of foo(const foo&) (in the bar module) is deserialized
this definition is ‘interesting’ and added to the ASTReader’s InterestingDecls
The end of the Deserializing context (8) is reached
In the non-modular-codegen case, DeclMustBeEmitted is not true for foo(const foo&) and it is shelved for lazy emission, end of story1. In the modular-codegen case, foo(const foo&) in the bar module must be emitted (all inline functions (implicitly or explicitly) defined in a module are emitted weak, etc)
So foo(const foo&) is non-lazily deserialized and emitted
The bar module’s DeclCursor is used for this, unprotected
execution eventually gets back to the deserialization of bar() - and the DeclCursor it’s using is out of position → badness.
Maybe there’s a better way to provide this timeline, I’m not sure - hopefully it makes sense.

Essentially the way ASTReader::ReadDeclRecord (started in (5) that contains the SavedStreamPosition and Deserializing context) seems to assume that the only things that will be non-lazily deserialized will come from the same module. Modular codegen breaks that invariant at the moment (well, with the patch provided).

I did try SavedStreamPosition-protecting the call from 3->4 (specifically the “Visit(Expr)” call in ReadStmtFromStream) though still hit some crashes. Maybe that’s the right path to go down still, but need to do more?

When I tried to copy the DeclCursor in ReadStmtFromStream that actually broke things pretty significantly (compile errors on valid code even without modular codegen enabled). But I don’t know much about the cursors - evidently more than only an efficiency concern, I guess the non-offset state in the copy of the cursor changes and so the original DeclCursor isn’t updated, etc. (I wonder about splitting these cursors into a shared state + offset, share the state with shared_ptr and make it cheap to copy the actual offset state around so there’s less reason to risk these sharing situations & all the SavedStreamPosition protection that requires).

Long & short of it: What should I do here? What’re the likely goals I should be trying to move the code towards, if any?

Dave

modular-codegen.diff (10.3 KB)

Richard_Smith · February 24, 2017, 10:22pm

We should have at least one instance of Deserializing extant during all
deserialization. It looks like one is missing from GetExternalDeclStmt.

I think it's correct that ReadStmtFromStream does not try to maintain the
stream position: it's intending to be called with the DeclsCursor pointing
at the Stmt to read, and intends to leave the stream pointing to the record
after that point. The external caller (GetExternalDeclStmt) jumps to the
correct bit location (as do all other users of the DeclsCursor) before
calling it, and does not expect to ever be called reentrantly. But a
reentrant call to GetExternalDeclStmt is exactly what's happening in your
case, because it also doesn't /defend/ against reentrancy from
end-of-deserialization actions through a Deserializing object.

dblaikie · February 24, 2017, 10:25pm

OK, sounds fair - so GetExternalDeclStmt needs at least a Deserializing object to defer deserialization further. Does it also need a SavedStreamPosition, do you think? I can’t immediately think of how/why it might, but they seemed to be paired in other places.

Richard_Smith · February 25, 2017, 1:13am

OK, sounds fair - so GetExternalDeclStmt needs at least a Deserializing
object to defer deserialization further. Does it also need a
SavedStreamPosition, do you think? I can't immediately think of how/why it
might, but they seemed to be paired in other places.

I think this is an accurate summary:
* You need a Deserializing object if you're going to deserialize (or more
accurately, temporarily violate invariants, use global cursors, ...) and
your caller might not be deserializing
* You need a SavedStreamPosition object if you're going to use a global
cursor and your caller might be deserializing
... so you'd see a pair of them whenever someone is using a global cursor
and doesn't know whether their caller is deserializing.

I think GetExternalDeclStmt doesn't need a SavedStreamPosition, but should
assert that NumCurrentElementsDeserializing == 0 before it creates its
Deserializing object.

dblaikie · February 27, 2017, 7:11pm

Reflected on this a bit more & realized that modular codegen shouldn’t’ve been triggering this case anyway - it could only happen (I think) if a modular codegen decl was found dynamically while deserializing otehr things. That means the modular codegen decl wasn’t in the modular codegen decls list.

So I changed the way the list is built (to not use isRequiredDecl, but to add things to the list in AddFunctionDefinition, right where the bit is set on the decl itself - so the list and the decl bits should never be out of sync) which avoids the need for other fixes.

Would it be worth checking in the missing Deserializing context and suggested assertion on principle anyway?

Richard_Smith · February 27, 2017, 8:42pm

Reflected on this a bit more & realized that modular codegen shouldn't've
been triggering this case anyway - it could only happen (I think) if a
modular codegen decl was found dynamically while deserializing otehr
things. That means the modular codegen decl wasn't in the modular codegen
decls list.

So I changed the way the list is built (to not use isRequiredDecl, but to
add things to the list in AddFunctionDefinition, right where the bit is set
on the decl itself - so the list and the decl bits should never be out of
sync) which avoids the need for other fixes.

Would it be worth checking in the missing Deserializing context and
suggested assertion on principle anyway?

It seems like we could get to the same issue any time loading a statement
transitively adds a deferred action that results in loading another
statement. I don't know if there are other codepaths that result in that
happening, but it seems like that should be possible (without resulting in
this issue), and we probably mostly avoid it because we usually load
statements lazily.

dblaikie · March 7, 2017, 1:13am

If you could provide some pointers I’d be willing to go hunting for a test case, but in leiu of that, is this the patch you had in mind:

diff --git lib/Serialization/ASTReader.cpp lib/Serialization/ASTReader.cpp
index c6564d666b…648c7b48ed 100644
— lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -6811,6 +6811,9 @@ Stmt *ASTReader::GetExternalDeclStmt(uint64_t Offset) {
// Offset here is a global offset across the entire chain.
RecordLocation Loc = getLocalBitOffset(Offset);
Loc.F->DeclsCursor.JumpToBit(Loc.Offset);

assert(NumCurrentElementsDeserializing == 0 &&
“should not be called while already deserializing”);
Deserializing D(this);
return ReadStmtFromStream(*Loc.F);
}

Happy to commit that with or without tests (if I can find tests) on current ToT as a defensive measure even though it turned out I didn’t need that.

Richard_Smith · March 8, 2017, 11:30pm

Reflected on this a bit more & realized that modular codegen shouldn't've
been triggering this case anyway - it could only happen (I think) if a
modular codegen decl was found dynamically while deserializing otehr
things. That means the modular codegen decl wasn't in the modular codegen
decls list.

So I changed the way the list is built (to not use isRequiredDecl, but to
add things to the list in AddFunctionDefinition, right where the bit is set
on the decl itself - so the list and the decl bits should never be out of
sync) which avoids the need for other fixes.

Would it be worth checking in the missing Deserializing context and
suggested assertion on principle anyway?

It seems like we could get to the same issue any time loading a statement
transitively adds a deferred action that results in loading another
statement. I don't know if there are other codepaths that result in that
happening, but it seems like that should be possible (without resulting in
this issue), and we probably mostly avoid it because we usually load
statements lazily.

If you could provide some pointers I'd be willing to go hunting for a test
case, but in leiu of that, is this the patch you had in mind:

diff --git lib/Serialization/ASTReader.cpp lib/Serialization/ASTReader.cpp
index c6564d666b..648c7b48ed 100644
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -6811,6 +6811,9 @@ Stmt *ASTReader::GetExternalDeclStmt(uint64_t
Offset) {
   // Offset here is a global offset across the entire chain.
   RecordLocation Loc = getLocalBitOffset(Offset);
   Loc.F->DeclsCursor.JumpToBit(Loc.Offset);
+ assert(NumCurrentElementsDeserializing == 0 &&
+ "should not be called while already deserializing");
+ Deserializing D(this);
   return ReadStmtFromStream(*Loc.F);
}
Happy to commit that with or without tests (if I can find tests) on
current ToT as a defensive measure even though it turned out I didn't need
that.

That's what I had in mind, yes.

dblaikie · March 9, 2017, 12:09am

r297322