This is an archive of the discontinued LLVM Phabricator instance.

clang-cl: Take dllexport from original function decl into account
ClosedPublic

Authored by sberg on Jan 27 2016, 7:49 AM.

Details

Summary

...in cases where a member function is redeclared as a friend of a nested class. (LibreOffice happens to get tripped up by this.)

Diff Detail

Event Timeline

sberg updated this revision to Diff 46138.Jan 27 2016, 7:49 AM
sberg retitled this revision from to clang-cl: Take dllexport from original function decl into account.
sberg updated this object.
sberg added a reviewer: rnk.
sberg added a subscriber: cfe-commits.
rnk edited reviewers, added: hans; removed: rnk.Jan 27 2016, 8:31 AM
rnk added a subscriber: rnk.

Hans knows all about dllexport now.

hans edited edge metadata.Jan 27 2016, 9:43 AM

Hi Stephan,

I would rather see that we could get this right in the AST.

The problem is that the Befriended::func() definition doesn't have dllexport attached:

`-CXXMethodDecl 0x5ba1cf0 parent 0x5b4f288 prev 0x5b4f750 <line:7:1, col:26> col:18 used func 'void (void)'
  `-CompoundStmt 0x5ba1de0 <col:25, col:26>

If I drop the friend declaration, it looks like this:

`-CXXMethodDecl 0x6af2a08 parent 0x6aa0288 prev 0x6aa04c0 <line:7:1, col:26> col:18 used func 'void (void)'
  |-CompoundStmt 0x6af2b30 <col:25, col:26>
  `-DLLExportAttr 0x6af2b20 <line:1:19> Inherited

It's a tricky situation though. I suspect what's happening is that when we build the AST for the friend decl, we haven't yet propagated the dllexport attribute from the 'Befriended' class to 'func'.

If the attribute was put directly on 'func', the friend declaration would pick it up:

struct  Befriended {
  static void __declspec(dllexport) func();
  struct Befriending {
    friend void Befriended::func();
  };
};
void Befriended::func() {}

|-CXXRecordDecl 0x6fe4288 </tmp/a.cc:1:1, line:6:1> line:1:9 struct Befriended definition
| |-CXXRecordDecl 0x6fe43a0 <col:1, col:9> col:9 implicit struct Befriended
| |-CXXMethodDecl 0x6fe4480 <line:2:3, col:42> col:37 func 'void (void)' static
| | `-DLLExportAttr 0x6fe4528 <col:26>
| `-CXXRecordDecl 0x6fe4560 <line:3:3, line:5:3> line:3:10 struct Befriending definition
|   |-CXXRecordDecl 0x6fe4670 <col:3, col:10> col:10 implicit struct Befriending
|   `-FriendDecl 0x6fe4868 <line:4:5, col:34> col:29
|     `-CXXMethodDecl 0x6fe4740 parent 0x6fe4288 prev 0x6fe4480 <col:5, col:34> col:29 func 'void (void)'
|       `-DLLExportAttr 0x6fe4858 <line:2:26> Inherited
`-CXXMethodDecl 0x6fe48c8 parent 0x6fe4288 prev 0x6fe4740 <line:7:1, col:26> col:18 func 'void (void)'
  |-CompoundStmt 0x7036710 <col:25, col:26>
  `-DLLExportAttr 0x6fe49e0 <line:2:26> Inherited

We need to figure out the ordering of applying the dllexport class attribute vs. parsing inner classes.

Yeah, my first naive finding when encountering the error was that it went away when unconditionally using FD->getCanonicalDecl() instead of FD in that if-else-if block. But that caused other parts of clang-test to fail. The current version passes all tests (happens to), but indeed does not feel "right" at all.

sberg updated this revision to Diff 137374.Mar 7 2018, 6:17 AM

Turns out DLLAttr-inherited-from-class is only added to members during Sema::CheckCompletedClass -> Sema::checkClassLevelDLLAttribute, when friend re-decls of those members may already have been created.

rnk accepted this revision.Mar 7 2018, 5:18 PM

lgtm

This revision is now accepted and ready to land.Mar 7 2018, 5:18 PM
This revision was automatically updated to reflect the committed changes.