Page MenuHomePhabricator

Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)
ClosedPublic

Authored by hans on Jul 31 2019, 6:52 AM.

Details

Summary

This is similar to r245139, but that only addressed dllexported classes. It was still possible to run into the same problem with dllexported members in an otherwise normal class (see bug). This uses the same strategy to fix: delay defining the method until the whole class has been parsed.

(The easiest way to see the ordering problem is in Parser::ParseCXXMemberSpecification(): it calls ParseLexedMemberInitializers() *after* ActOnFinishCXXMemberDecls(), which was trying to define the dllexport method. Now we delay it to ActOnFinishCXXNonNestedClass() which is called after both of those.)

Diff Detail

Repository
rL LLVM

Event Timeline

hans created this revision.Jul 31 2019, 6:52 AM
rnk added inline comments.Jul 31 2019, 9:54 AM
clang/lib/Sema/SemaDeclCXX.cpp
11545 ↗(On Diff #212568)

Do we need to do this until fixpoint? Suppose a dllexported implicit special member triggers a template instantiation, and the template has a dllexported defaulted special member, something like:

struct Bar { Bar(); };
template <typename T> struct Foo { __declspec(dllexport) Foo() = default; Bar obj; };
struct Baz {
   ... not sure how to trigger instantiation
};
hans marked an inline comment as done.Jul 31 2019, 10:03 AM
hans added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
11545 ↗(On Diff #212568)

I think that should work, and that's why the function is written to be re-entrant by having a local worklist. If it triggers a template instantiation, ActOnFinishCXXNonNestedClass should get called for the newly instantiated class. But I'm also not sure how to write a test case that would trigger it.

rnk accepted this revision.Jul 31 2019, 10:48 AM

lgtm

clang/lib/Sema/SemaDeclCXX.cpp
11545 ↗(On Diff #212568)

Right, I see. Nevermind then.

This revision is now accepted and ready to land.Jul 31 2019, 10:48 AM
aganea added inline comments.Jul 31 2019, 11:42 AM
clang/lib/Sema/SemaDeclCXX.cpp
11545 ↗(On Diff #212568)

This works on latest MSVC:

$ cat test.cc
struct Bar { Bar(); };
template <typename T> struct Foo { __declspec(dllexport) Foo() = default; Bar b; };
template class Foo<int>;

$ cl /c /Z7 test.cc   <-- crashes if using clang-cl

$ lib test.obj /def

$ cat test2.cc
struct Bar { Bar() { } };
template <typename T> struct Foo { __declspec(dllimport) Foo() = default; Bar b; };
int main() {
  Foo<int> f;
  return 0;
}

$ cl /c /Z7 test2.cc

$ link test.lib test2.cc /DEBUG
This revision was automatically updated to reflect the committed changes.
hans marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 1:00 AM
hans added a comment.Tue, Nov 5, 7:15 AM

Sorry, seems I forgot to submit this comment..

clang/lib/Sema/SemaDeclCXX.cpp
11545 ↗(On Diff #212568)

Thanks! I believe that's a separate issue, though, and not related to this patch. I've filed https://bugs.llvm.org/show_bug.cgi?id=42857 for it.