This revision was automatically updated to reflect the committed changes.
Closed by commit rC346069: Add /Zc:DllexportInlines option to clang-cl
(authored by tikuta, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D51340?vs=172348=172485#toc
Repository:
rC Clang
takuto.ikuta added a comment.
Thank you! I'll submit this if other people does not have other comments.
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:9
+// RUN: -emit-llvm -O1 -o - | \
+// RUN: FileCheck
takuto.ikuta updated this revision to Diff 172348.
takuto.ikuta marked an inline comment as done.
takuto.ikuta added a comment.
Fix check-prefix
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.
Assuming the test still passes when you put the EXPORTINLINE filecheck prefix
back, looks good to me!
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:9
+//
takuto.ikuta added a comment.
Thank you for quick review!
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5715
+ (MD->isThisDeclarationADefinition() ||
+ MD->isImplicitlyInstantiable()) &&
+ TSK != TSK_ExplicitInstantiationDeclaration &&
takuto.ikuta updated this revision to Diff 172332.
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Driver/CLCompatOptions.td
takuto.ikuta updated this revision to Diff 172331.
takuto.ikuta added a comment.
added a few more check
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
clang/include/clang/Driver/CC1Options.td
takuto.ikuta updated this revision to Diff 172330.
takuto.ikuta marked 10 inline comments as done.
takuto.ikuta added a comment.
Address comment
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
hans added a comment.
Thanks! I don't think the new isThisDeclarationADefinition() and
isImplicitlyInstantiable() checks are right. Besides that, I only have a few
more comments about the test.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5715
+
takuto.ikuta marked an inline comment as done.
takuto.ikuta added a comment.
I missed the comment about driver flag test.
https://reviews.llvm.org/D51340
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
takuto.ikuta updated this revision to Diff 172317.
takuto.ikuta added a comment.
Added cl driver flag test
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
clang/include/clang/Driver/CC1Options.td
takuto.ikuta added a comment.
Hans, thank you for review! I addressed all your comment and fixed small
behavior.
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:76
+ // DEFAULT-NOT: InlineOutclassDefFunc@NoTemplateExportedClass@@
+ __forceinline void
takuto.ikuta updated this revision to Diff 172304.
takuto.ikuta marked 17 inline comments as done.
takuto.ikuta added a comment.
Simplify test and fix some behavior.
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
hans added inline comments.
Comment at: clang/include/clang/Driver/CLCompatOptions.td:336
HelpText<"Disable precompiled headers, overrides /Yc and /Yu">;
+def _SLASH_Zc_dllexportInlines : CLFlag<"Zc:dllexportInlines">;
+def _SLASH_Zc_dllexportInlines_ :
hans added a comment.
Thank you Takuto! I think the functionality looks great now: it's not too
complicated :-)
I only have comments about the test.
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:3
+// RUN: -fno-dllexport-inlines -emit-llvm -O0 -o -
takuto.ikuta added a comment.
Thank you for review!
Comment at: clang/include/clang/Basic/LangOptions.h:246
+ /// If set, dllexported classes dllexport their inline methods.
+ bool DllExportInlines = true;
+
rnk wrote:
> We should define this in the
takuto.ikuta updated this revision to Diff 172090.
takuto.ikuta marked 2 inline comments as done.
takuto.ikuta added a comment.
Added option to LangOptions.def
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.def
rnk added inline comments.
Comment at: clang/include/clang/Basic/LangOptions.h:246
+ /// If set, dllexported classes dllexport their inline methods.
+ bool DllExportInlines = true;
+
We should define this in the LangOptions.def file.
rnk added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > >
takuto.ikuta marked an inline comment as done.
takuto.ikuta added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
hans
takuto.ikuta updated this revision to Diff 171916.
takuto.ikuta added a comment.
export/import explicit template instantiation function
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
hans added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > >
takuto.ikuta added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
hans wrote:
> takuto.ikuta wrote:
> > takuto.ikuta
hans added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
takuto.ikuta wrote:
> takuto.ikuta wrote:
> > hans wrote:
> > >
takuto.ikuta added a comment.
Thank you for quick fix!
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
takuto.ikuta wrote:
> hans wrote:
takuto.ikuta updated this revision to Diff 171877.
takuto.ikuta added a comment.
Rebased to take r345699
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
hans added inline comments.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+if (ClassExported) {
hans wrote:
> takuto.ikuta wrote:
> >
hans added a comment.
>> $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o
>> lib.so lib.cc
>> $ g++ main.cc lib.so
>> /tmp/cc557J3i.o: In function `S::bar()':
>> main.cc:(.text._ZN1S3barEv[_ZN1S3barEv]+0xd): undefined reference to
>> `foo()'
>>
>>
>> So this is a
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1278799, @hans wrote:
> I've been thinking more about your example with static locals in lambda and
> how this works with regular dllexport.
>
> It got me thinking more about the problem of exposing inline functions from a
>
hans added inline comments.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+if (ClassExported) {
takuto.ikuta wrote:
> takuto.ikuta wrote:
hans added a comment.
I've been thinking more about your example with static locals in lambda and how
this works with regular dllexport.
It got me thinking more about the problem of exposing inline functions from a
library. For example:
`lib.h`:
#ifndef LIB_H
#define LIB_H
int
takuto.ikuta updated this revision to Diff 171466.
takuto.ikuta marked an inline comment as done.
takuto.ikuta added a comment.
Added explanation comment for added attributes and rebased
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
takuto.ikuta added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:9552
+// overwrite linkage of explicit template instantiation
+// definition/declaration.
+return GVA_DiscardableODR;
hans wrote:
> Can you give an example for why this is
hans added a comment.
In https://reviews.llvm.org/D51340#1266312, @takuto.ikuta wrote:
> Hans, I addressed all your comments.
> How do you think about current implementation?
Just a few questions, but I think it's pretty good.
Comment at:
takuto.ikuta added a comment.
ping? Can I go forward in this way?
https://reviews.llvm.org/D51340
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
takuto.ikuta added a comment.
Herald added a subscriber: nhaehnle.
Hans, I addressed all your comments.
How do you think about current implementation?
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK !=
takuto.ikuta updated this revision to Diff 169792.
takuto.ikuta added a comment.
remove comment out code
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
takuto.ikuta updated this revision to Diff 169791.
takuto.ikuta added a comment.
Fix linkage for inline function of explicit template instantiation declaration
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
takuto.ikuta updated this revision to Diff 169783.
takuto.ikuta added a comment.
Remove unnecessary attr creation
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
takuto.ikuta updated this revision to Diff 169652.
takuto.ikuta retitled this revision from "[WIP] Add /Zc:DllexportInlines option
to clang-cl" to "Add /Zc:DllexportInlines option to clang-cl".
takuto.ikuta added a comment.
Export function inside explicit template instantiation definition
takuto.ikuta added inline comments.
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+while (FD && !getDLLAttr(FD) &&
+ !FD->hasAttr() &&
hans wrote:
> takuto.ikuta wrote:
> > hans wrote:
> > > takuto.ikuta wrote:
> > > > hans wrote:
> > > > > Why
takuto.ikuta updated this revision to Diff 169365.
takuto.ikuta added a comment.
Address comment
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
hans added inline comments.
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+while (FD && !getDLLAttr(FD) &&
+ !FD->hasAttr() &&
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > > hans wrote:
> > > > Why does this need to be a loop? I
takuto.ikuta added inline comments.
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+while (FD && !getDLLAttr(FD) &&
+ !FD->hasAttr() &&
hans wrote:
> takuto.ikuta wrote:
> > hans wrote:
> > > Why does this need to be a loop? I don't think
hans added inline comments.
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+while (FD && !getDLLAttr(FD) &&
+ !FD->hasAttr() &&
takuto.ikuta wrote:
> hans wrote:
> > Why does this need to be a loop? I don't think FunctionDecl's can be nested?
>
takuto.ikuta added a comment.
Thank you for review! I updated the code.
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+while (FD && !getDLLAttr(FD) &&
+ !FD->hasAttr() &&
hans wrote:
> Why does this need to be a loop? I don't think
takuto.ikuta updated this revision to Diff 169179.
takuto.ikuta marked an inline comment as done.
takuto.ikuta added a comment.
address comment
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
hans added a comment.
Thanks! I think this is getting close now.
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+while (FD && !getDLLAttr(FD) &&
+ !FD->hasAttr() &&
Why does this need to be a loop? I don't think FunctionDecl's can be nested?
takuto.ikuta added a comment.
Thank you for review!
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5599
+bool Sema::isInlineFunctionDLLExportable(const CXXMethodDecl *MD) {
+ assert(MD->isInlined());
hans wrote:
> Okay, breaking out this logic is a little
takuto.ikuta updated this revision to Diff 168979.
takuto.ikuta added a comment.
address comment
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
hans added inline comments.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5599
+bool Sema::isInlineFunctionDLLExportable(const CXXMethodDecl *MD) {
+ assert(MD->isInlined());
Okay, breaking out this logic is a little better, but I still don't like that
we now
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1256299, @hans wrote:
> In https://reviews.llvm.org/D51340#1254898, @takuto.ikuta wrote:
>
> > Ping?
>
>
> Sorry, I know I'm being slow to look at this :-( I have not forgotten, it's
> just a lot of other things happening at the
hans added a comment.
In https://reviews.llvm.org/D51340#1254898, @takuto.ikuta wrote:
> Ping?
Sorry, I know I'm being slow to look at this :-( I have not forgotten, it's
just a lot of other things happening at the same time.
https://reviews.llvm.org/D51340
takuto.ikuta added a comment.
Ping?
https://reviews.llvm.org/D51340
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
takuto.ikuta added inline comments.
Comment at: clang/lib/Sema/SemaDecl.cpp:12624
+ isa(D)) {
+CXXMethodDecl *MD = dyn_cast(D);
+CXXRecordDecl *Class = MD->getParent();
hans wrote:
> Hmm, now we're adding an AST walk over all inline methods which
takuto.ikuta added a comment.
Thank you for review!
In https://reviews.llvm.org/D51340#1246508, @hans wrote:
> The static local stuff still makes me nervous. How much does Chromium break
> if we just ignore the problem? And how does -fvisibility-inlines-hidden
> handle the issue?
I'm not
takuto.ikuta updated this revision to Diff 167683.
takuto.ikuta added a comment.
Update comment for Sema::ActOnFinishInlineFunctionDef
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
takuto.ikuta updated this revision to Diff 167682.
takuto.ikuta added a comment.
Extract inline function export check to function
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
hans added a comment.
The static local stuff still makes me nervous. How much does Chromium break if
we just ignore the problem? And how does -fvisibility-inlines-hidden handle the
issue?
Also, Sema already has a check for static locals in C inline functions:
$ echo "inline void f() {
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1243453, @hans wrote:
> In https://reviews.llvm.org/D51340#1243331, @takuto.ikuta wrote:
>
> > Ping?
> >
> > This patch reduced obj size largely, and I expect this makes distributed
> > build (like goma) faster by reducing data
hans added a comment.
In https://reviews.llvm.org/D51340#1243331, @takuto.ikuta wrote:
> Ping?
>
> This patch reduced obj size largely, and I expect this makes distributed
> build (like goma) faster by reducing data transferred on network.
I'll try to look at it this week.
Have you confirmed
takuto.ikuta added a comment.
Ping?
This patch reduced obj size largely, and I expect this makes distributed build
(like goma) faster by reducing data transferred on network.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5244
+ false))
+
takuto.ikuta updated this revision to Diff 166450.
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Driver/CLCompatOptions.td
clang/lib/Driver/ToolChains/Clang.cpp
takuto.ikuta updated this revision to Diff 166448.
takuto.ikuta edited the summary of this revision.
takuto.ikuta added a comment.
Remove unnecessary willHaveBody check condition
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Basic/LangOptions.h
takuto.ikuta added a comment.
PTAL again.
I confirmed that current patch can link chrome and functions with local static
variable are exported.
But current ToT clang was not improved well by this patch.
I guess there is some change recently making effect of this patch smaller. Or
chromium has
takuto.ikuta updated this revision to Diff 166087.
takuto.ikuta retitled this revision from "[WIP] Add /Zc:DllexportInlines option
to clang-cl" to "Add /Zc:DllexportInlines option to clang-cl".
takuto.ikuta edited the summary of this revision.
https://reviews.llvm.org/D51340
Files:
takuto.ikuta added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5244
+ false))
+CmdArgs.push_back("-fvisibility-inlines-hidden");
+
hans wrote:
> takuto.ikuta wrote:
> > hans wrote:
> > > Huh, does this actually affect
takuto.ikuta updated this revision to Diff 164822.
takuto.ikuta added a comment.
I'm trying to handle local static var correctly.
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Driver/CLCompatOptions.td
clang/include/clang/Sema/Sema.h
clang/lib/Driver/ToolChains/Clang.cpp
hans added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5244
+ false))
+CmdArgs.push_back("-fvisibility-inlines-hidden");
+
takuto.ikuta wrote:
> hans wrote:
> > Huh, does this actually affect whether functions get
takuto.ikuta added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5244
+ false))
+CmdArgs.push_back("-fvisibility-inlines-hidden");
+
hans wrote:
> Huh, does this actually affect whether functions get dllexported or not?
hans added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5244
+ false))
+CmdArgs.push_back("-fvisibility-inlines-hidden");
+
Huh, does this actually affect whether functions get dllexported or not?
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1226989, @takuto.ikuta wrote:
> In https://reviews.llvm.org/D51340#1222013, @hans wrote:
>
> > Did both your builds use PCH? It'd be interesting to see the difference
> > without PCH too; the effect should be even larger.
>
>
>
takuto.ikuta updated this revision to Diff 164379.
takuto.ikuta edited the summary of this revision.
https://reviews.llvm.org/D51340
Files:
clang/include/clang/Driver/CLCompatOptions.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Sema/SemaDeclCXX.cpp
takuto.ikuta marked 2 inline comments as done.
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1222013, @hans wrote:
> Did both your builds use PCH? It'd be interesting to see the difference
> without PCH too; the effect should be even larger.
Added stats of without PCH
takuto.ikuta marked 4 inline comments as done.
takuto.ikuta added inline comments.
Comment at: clang/include/clang/Basic/LangOptions.def:117
LANGOPT(Digraphs , 1, 0, "digraphs")
+LANGOPT(DllexportInlines , 1, 1, "If dllexport a class should dllexport
implicit inline
takuto.ikuta updated this revision to Diff 164353.
takuto.ikuta edited the summary of this revision.
takuto.ikuta added a comment.
Herald added a subscriber: dschuff.
Make patch closer to Nico's original implementation, but warns local static
variable instead of detecting it.
In
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1225805, @takuto.ikuta wrote:
> I found that current ToT with original Nico's patch does not allow to link
> ui_base.dll
>
> https://github.com/atetubou/llvm-project-20170507/tree/totwin_dbg_1236_nico
> gives the following link
takuto.ikuta added a comment.
I found that current ToT with original Nico's patch does not allow to link
ui_base.dll
https://github.com/atetubou/llvm-project-20170507/tree/totwin_dbg_1236_nico
gives the following link error
https://pastebin.com/9LVRbRVn
I will do bisection to find when some
probinson added subscribers: cfe-commits, probinson.
probinson added a comment.
+cfe-commits
https://reviews.llvm.org/D51340
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
79 matches
Mail list logo
