[cfe-dev] Modular Codegen: Cross-module deserialization problem
David Blaikie via cfe-dev
cfe-dev at lists.llvm.org
Wed Mar 8 16:09:15 PST 2017
r297322
On Wed, Mar 8, 2017 at 3:31 PM Richard Smith <richard at metafoo.co.uk> wrote:
> On 6 March 2017 at 17:13, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Mon, Feb 27, 2017 at 12:43 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
>
> On 27 February 2017 at 11:11, David Blaikie <dblaikie at gmail.com> wrote:
>
> 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.
>
> On Fri, Feb 24, 2017 at 5:13 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
>
> On 24 February 2017 at 14:25, David Blaikie via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> 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.
>
>
> On Fri, Feb 24, 2017 at 2:22 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
>
> On 24 February 2017 at 09:41, David Blaikie <dblaikie at gmail.com> wrote:
>
> 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?
>
>
> 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.
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170309/80db5537/attachment.html>
More information about the cfe-dev
mailing list