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&id=172485#toc
Repository:
rC Clang
https:/
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 --che
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
clang/include/clang/Driver/CC
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
clang/lib/Driver/ToolChains/Clang.
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
clang/include/clang/Driver/
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
clang/include/clang/Driver/CC
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
+ (MD->isThisDeclarationA
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
http://lists.llvm.org/cgi-bin/mailman/l
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
clang/include/clang/Driv
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 Inl
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
clang/in
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_ : CLFlag<"Zc:dllexportInl
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 LangOptio
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
clang/include/
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.
https://reviews.llvm.org/
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:
> > > h
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 wrot
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
clang/include/clang/Driver/CC1Options.td
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 wrote
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
clang/include/clang/Driver/C
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
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
> libra
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 foo();
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
clang/include/clang/Basic/LangO
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 ne
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: clang/include/clang/Basic/Attr.td:2
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 != 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
clang/include/clang/Driver/C
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
clang/include/cl
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
clang/include/clang
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
https
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 do
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
clang/include/clang/Driver/CLCompatO
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 d
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 FunctionD
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?
> Thi
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 FunctionDecl'
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
clang/include/clang/Driver/CC1Op
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 better
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
clang/include/clang/Driver/CLCompatO
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 ha
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 sa
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 pro
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 sur
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
clang/include/clang/Driver/CLCompa
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
clang/include/clang/Driver/CLCompatOpti
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() { static
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 tra
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))
+CmdArgs.push_
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
clang/lib/Frontend/CompilerInvoc
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
clang/include/clang/Driver/CC1Opt
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:
clang/inclu
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
cl
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 dlle
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?
https://reviews.llvm
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.
>
>
> Add
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
clang/test/CodeGenCXX/dllexport-no
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 build.
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 m
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 checkClassLevelDL
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 er
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 inl
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
