[cfe-dev] Modular Codegen: Cross-module deserialization problem

[David Blaikie via cfe-dev]

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 <typename T>
  foo bar(foo &f) {
    return f;
  }
  void z() { (void)&bar<int>; }

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)

   1. bar() body deserialization begins
   2. bar module's DeclCursor is used, jumping to the start of the bar()'s
   definition
   3. clang::ASTReader::ReadStmtFromStream iterates through Stmts in bar()
   no SavedStreamPosition nor Deserializing object here
   4. deserializing the EXPR_CXX_CONSTRUCT gets interesting:
   5. eventually involves deserializing foo(const foo&)
   6. foo module's DeclCursor is used
   7. foo module's DeclCursor is saved/preserved with a SavedStreamPosition
   8. a ExternalASTSource::Deserializing context is started
   9. the definition of foo(const foo&) (in the bar module) is deserialized
   10. this definition is 'interesting' and added to the ASTReader's
   InterestingDecls
   11. The end of the Deserializing context (8) is reached
      1. In the non-modular-codegen case, DeclMustBeEmitted is not true for
      foo(const foo&) and it is shelved for lazy emission, end of story
   12. 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)
   13. So foo(const foo&) is non-lazily deserialized and emitted
   14. The bar module's DeclCursor is used for this, unprotected
   15. 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170224/c6f733d6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: modular-codegen.diff
Type: application/octet-stream
Size: 10559 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170224/c6f733d6/attachment.obj>