[cfe-dev] [lldb-dev] stable layout bug for imported record decls.

[Gábor Márton via cfe-dev]

Hi Lang, Alexey,

I dug deeper into this and it seems like the issue happens only when a
**minimal** import is used. LLDB uses the minimal import. CrossTU
static analyzer uses the normal mode.
In normal import mode, in `ImportDeclContext` we do import all the
methods in the correct order. However, in minimal mode we early return
before importing the methods.
So by merging D44100 this particular problem will not be solved.
Still, that patch sounds very reasonable and I support that we should
reorder all imported Decls, not just fields.

One trivial solution would be to change `ImporDeclContext` to behave
the same way in both modes. This is somewhat similar to the "brute
force" method you mentioned.
I am not an LLDB expert, so I am not sure if this is acceptable, and
really don't know how many LLDB tests would it break, but this seems
the simplest solution (and preferred) to me.

The other solution if you'd like to keep the minimal behavior is the
index based solution (as you mentioned).
You should compare the order of all the imported methods (and fields)
to the order in the original class in the "From" context. And I
believe you have to do that at the end of VisitFunctionDecl. It would
not work if you did that check when the type become complete, since in
minimal mode we never set the class to be incomplete.

I have created a unit test case, which fails in minimal mode and
succeeds in normal mode. You can change the mode in
`ASTImporterTestBase::TU::lazyInitImporter`.
If you provide a patch then could you please also add this test (or
similar) for both normal and minimal mode?
```
TEST_P(ASTImporterTestBase, OrderOfVirtualMethods) {
  auto Code =
      R"(
      class Base {
      public:
        virtual void bar() {}
        virtual void foo() {}
      };

      class Derived : public Base {
      public:
        void foo() override {}
      };
      )";
  Decl *FromTU = getTuDecl(Code, Lang_CXX11, "input0.cc");
  auto *DerivedFoo = FirstDeclMatcher<FunctionDecl>().match(
      FromTU, functionDecl(hasName("foo"),
                           hasParent(cxxRecordDecl(hasName("Derived")))));
  auto *BaseBar = FirstDeclMatcher<FunctionDecl>().match(
      FromTU, functionDecl(hasName("bar"),
                           hasParent(cxxRecordDecl(hasName("Base")))));

  Import(DerivedFoo, Lang_CXX);
  // Importing Base::bar explicitly is needed only in minimal mode,
  // in normal mode we already imported all methods of Base.
  Import(BaseBar, Lang_CXX);

  Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
  auto *ImportedBase = FirstDeclMatcher<CXXRecordDecl>().match(
      ToTU, cxxRecordDecl(hasName("Base")));
  MatchVerifier<Decl> Verifier;
  EXPECT_TRUE(Verifier.match(ImportedBase,
                             cxxRecordDecl(hasMethodOrder({"bar", "foo"}))));
}
```

Thanks,
Gabor
On Fri, Aug 10, 2018 at 3:47 PM Alexey Sidorin <alexey.v.sidorin at ya.ru> wrote:
>
> (+ Gabor and Gabor)
>
> Hi Lang,
>
> We faced a very similar issue with record fields where import order can change the order of imported FieldDecls resulting in broken ASTRecordLayout. The patch for this issue is on review: https://reviews.llvm.org/D44100. It just reorders the fields after structure import is finished. CSA guys also reported the same problem with FriendDecls in the same review.The order of methods was not a problem for us but your report adds a new item to support. It looks like _all_ decls inside RecordDecl have to be reordered. I'll try to resurrect the patch this weekend (it is a bit outdated due to my workload, sorry) and add you as a reviewer so you can check if it solves the problem or not.
>
> 09.08.2018 20:46, Lang Hames via lldb-dev пишет:
>
> Hi clang-dev, lldb-dev,
>
> It looks like my clang commit r305850, which modified ASTImporter to import method override tables from an external context, introduced a new bug which manifests as incorrect vtable layouts for LLDB expression code.
>
> The bug itself is fairly straightforward. In r305850 I introduced the following method, which is called from ASTNodeImporter::VisitFunctionDecl:
>
> void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
>                                       CXXMethodDecl *FromMethod) {
>   for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
>     ToMethod->addOverriddenMethod(
>       cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(
>                                             FromOverriddenMethod))));
> }
>
> This will produce the correct override table, but can also cause methods in the Base class to be visited in the wrong order. Consider:
>
> class Base {
> public:
>   virtual void bar() {}
>   virtual void foo() {}
> };
>
> class Derived : public Base {
> public:
>   void foo() override {}
> };
>
> If Derived is imported into a new context before Base, then the importer will visit Derived::foo, and (via ImportOverrides) immediately import Base::foo, but this happens before Base::bar is imported. As a consequence, the decl order on the imported Base class will end up being [ foo, bar ], instead of [ bar, foo ]. In LLDB expression evaluation this manifests as an incorrect vtable layout for Base, with foo occupying the first slot.
>
> I am looking for suggestions on the right way to fix this. A brute force solution might be to always have ASTNodeImporter::VisitRecordDecl visit all base classes, then all virtual methods, which would ensure they are visited in the original decl order. However I do not know whether this covers all paths by which a CXXRecordDecl might be imported, nor whether the performance of this solution would be acceptable (it seems like it would preclude a lot of laziness).
>
> Alternatively we might be able to associate an index with each imported decl and sort on that when we complete the type, but that would leave imported decls in the wrong order until the type was complete, and since I do not know all the use cases for the importer I'm concerned people may rely on the decl order before type is complete.
>
> Any insight from ASTImporter experts would be greatly appreciated. :)
>
> Cheers,
> Lang.
>
>
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>