[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk edited reviewers, added: hans, efriedma; removed: majnemer, rnk, aeubanks. rnk added a comment. Thanks for working on this, this is also an issue for our users. I've been out on leave. I replaced myself as a reviewer with Hans. CHANGES SINCE LAST ACTION

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. (I apologize it took me so long to get back to reviewing this.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Herald added subscribers: bd1976llvm, jplehr. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:520 + getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit)) +return; + efriedma wrote: > I don't really like the

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 502776. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. Would be nice to have feedback on this! So much work was put into it! @rnk, @efriedma CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits mailing list

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. ping? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 494051. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 493726. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 493411. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam marked an inline comment as done. zahiraam added a comment. Thanks. Please see my comments/questions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits mailing list

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam marked an inline comment as done. zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { efriedma wrote: > zahiraam wrote: > >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:520 + getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit)) +return; + I don't really like the copy-pasted bits here. Comment at:

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { efriedma wrote: > zahiraam wrote: > > efriedma wrote: > > > zahiraam wrote: > >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 486274. zahiraam marked 2 inline comments as done. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files:

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1926 +DiagnosticsEngine = CGM.getContext().getDiagnostics(); +Diags.Report(diag::warn_for_global_ctor_for_dllimport) << D; +return nullptr; I

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484840. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484814. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { efriedma wrote: > zahiraam wrote: > > efriedma wrote: > > > zahiraam wrote: > >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484665. zahiraam marked an inline comment as done. Herald added a subscriber: aheejin. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { zahiraam wrote: > efriedma wrote: > > zahiraam wrote: > > > Do you agree this

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam marked 3 inline comments as done. zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { efriedma wrote: > zahiraam wrote: > >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:254 +def warn_for_global_ctor_for_dllimport : Warning< + "global constructor is generated for dllimport global var %0">, Missing code to emit this warning?

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/test/CodeGenCXX/PR19955.cpp:20 +// CHECK-64: store ptr @"?var@@3HA", ptr @"?varp@@3PEAHEA" +// CHECK: ret void @rnk was concerned about varp pointing to dllimport var. That does it I think. CHANGES SINCE

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484278. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { Do you agree this should be done only when one of those flags is on? CHANGES

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484258. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam marked an inline comment as done. zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5009 + if (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor && NeedsGlobalDtor) { +EmitCXXCtorInit(D, GV, true, 201, llvm::StringLiteral("ctor"),

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852 + if (!CD->isTrivial() && !D->getTLSKind()) +NeedsGlobalCtor = true; +} zahiraam wrote: > efriedma wrote: > > zahiraam wrote: > > > efriedma wrote:

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852 + if (!CD->isTrivial() && !D->getTLSKind()) +NeedsGlobalCtor = true; +} efriedma wrote: > zahiraam wrote: > > efriedma wrote: > > > I have no idea

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852 + if (!CD->isTrivial() && !D->getTLSKind()) +NeedsGlobalCtor = true; +} zahiraam wrote: > efriedma wrote: > > I have no idea what this code is

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852 + if (!CD->isTrivial() && !D->getTLSKind()) +NeedsGlobalCtor = true; +} efriedma wrote: > I have no idea what this code is supposed to do. I have

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484062. zahiraam marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4761 + return Opts.CPlusPlus && D->hasConstantInitialization() && + D->getType()->isRecordType(); +} Not sure what the `D->getType()->isRecordType()` check is doing here.

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 484006. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. Right, the call to atexit from the constructor function shouldn't be there. I think you need to add a "skip emitting the destructor" flag to EmitCXXCtorInit CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. In D137107#3999681 , @efriedma wrote: > Not sure exactly what code that generates in its current form, but that's > roughly the right idea, yes. Thanks. This is the IR I am getting for this simple case. I think there is still

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. Not sure exactly what code that generates in its current form, but that's roughly the right idea, yes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. This uploaded patch is a draft (LIT tests fails but are not updated since I am not sure about these changes) that I am part way confident about. There are still a lot of things that I need to look at, but would you mind looking at it to see if this is moving in the

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 483320. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 481271. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 481029. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a subscriber: eli.friedman. zahiraam added a comment. @eli.friedman Thanks. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 480574. zahiraam marked 3 inline comments as done. zahiraam edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. (-Wglobal-constructors warning is still not implemented.) Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 479652. zahiraam marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGDeclCXX.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added a comment. To recap, I'm in favor of implementing dllimport constexpr global initializers with a high priority dynamic initializer. I would suggest using init_priority of 201 to run right after the "compiler" initializers. CHANGES SINCE LAST ACTION

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a reviewer: aeubanks. efriedma added a subscriber: aeubanks. efriedma added a comment. I'm not really familiar with the way constructor priorities work on Windows works... see https://reviews.llvm.org/D131910 ? Adding @aeubanks as a reviewer. CHANGES SINCE LAST ACTION

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } efriedma wrote: > rnk wrote: > > zahiraam

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } rnk wrote: > zahiraam wrote: > > efriedma

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } zahiraam wrote: > efriedma wrote: > > zahiraam

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } efriedma wrote: > zahiraam wrote: > >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. Looking at this again, I just thought of something: in C mode, we probably don't want to generate global constructors, so we might want to continue to print an error in that case. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 +

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam marked 3 inline comments as done. zahiraam added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + }

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } zahiraam wrote: > efriedma wrote: > > I don't

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } efriedma wrote: > I don't understand why this

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4822 Init = Initializer; + if (Initializer->isDLLImportDependent()) +NeedsGlobalCtor = true; This check probably needs to be inside tryEmitForInitializer, so

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. That's roughly what I was thinking, yes. The one missing piece is the code to modify the constructor priorities to make sure that "constant" variables get initialized first. (This ensures we honor the C++ rules for "constant initialization".)

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. In D137107#3931259 , @efriedma wrote: > There are multiple notions of "constant" here: > > - Whether a value is invariant at runtime. > - Whether a value is legal in a "constexpr" expression. > - Whether we can emit a constant

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 478624. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CodeGenModule.cpp clang/lib/Sema/SemaDecl.cpp clang/test/CodeGenCXX/PR19955.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. There are multiple notions of "constant" here: - Whether a value is invariant at runtime. - Whether a value is legal in a "constexpr" expression. - Whether we can emit a constant without emitting code that runs at startup to initialize it. The third is generally a

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added subscribers: efriedma, hans. rnk added a comment. Sorry for the delay, dev meeting. I think we should try to add some reviewers for IR gen to help, I don't have a lot of time to be responsive: https://github.com/llvm/llvm-project/blob/main/clang/CodeOwners.rst#clang-llvm-ir-generation

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. In D137107#3906766 , @rnk wrote: > In D137107#3905443 , @zahiraam > wrote: > >> extern int __declspec(dllimport) next(int n); >> int main () { >> extern int _declspec(dllimport)

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam updated this revision to Diff 473693. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 Files: clang/lib/Sema/SemaDecl.cpp clang/test/CodeGenCXX/PR19955.cpp clang/test/CodeGenCXX/dllimport.cpp

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added a comment. In D137107#3905443 , @zahiraam wrote: > extern int __declspec(dllimport) next(int n); > int main () { > extern int _declspec(dllimport) val; > constexpr int& val_ref = val; > int i = next(val_ref); > return i; >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

mstorsjo added a comment. In D137107#3906439 , @zahiraam wrote: > Here is an even smaller test case. > lib.cpp: > __declspec(dllexport) int val=12; > > t1.cpp: > int main () { > > extern int _declspec(dllimport) val; > int& val_ref = val; > return

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. In D137107#3906413 , @mstorsjo wrote: > In D137107#3905443 , @zahiraam > wrote: > >> and the same assembly (load __imp_?val) (is that why you made the comment >> above?). And the test

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

mstorsjo added a comment. In D137107#3905443 , @zahiraam wrote: > and the same assembly (load __imp_?val) (is that why you made the comment > above?). And the test case runs. Yes, pretty much - the actual value of the address of `` isn't known at

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. In D137107#3897568 , @mstorsjo wrote: > In D137107#3897326 , @rnk wrote: > >> Unless I'm missing something, I think Clang's behavior here is preferable to >> MSVC's. MSVC produces code

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

mstorsjo added a comment. In D137107#3897326 , @rnk wrote: > Unless I'm missing something, I think Clang's behavior here is preferable to > MSVC's. MSVC produces code which will not link. Clang turns the linker error > into a compiler error, which is

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam added a comment. In D137107#3897326 , @rnk wrote: > Unless I'm missing something, I think Clang's behavior here is preferable to > MSVC's. MSVC produces code which will not link. Clang turns the linker error > into a compiler error, which is

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added a subscriber: akhuang. rnk added a comment. @akhuang, can you help out with this? The address of dllimport symbols not being constexpr is a big pain point for our users too, and I'd love to fix it: https://github.com/protocolbuffers/protobuf/issues/10159 I am ready to be wrong here:

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added a comment. Unless I'm missing something, I think Clang's behavior here is preferable to MSVC's. MSVC produces code which will not link. Clang turns the linker error into a compiler error, which is generally easier for the user to understand. To my knowledge, it is still true that

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

zahiraam created this revision. zahiraam added reviewers: majnemer, rnk. Herald added a project: All. zahiraam requested review of this revision. Herald added a project: clang. Microsoft allows the support of ‘constexpr’ with ‘__declspec(dllimport) starting from VS2017 15.u update 4. See