[clang][ExprConstant] Fix error on static constexpr symbol in dllimport function #171628
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Alexandre Ganea (aganea) ChangesConsider the following: With clang-cl, this generates an error: The problem here is that the static variable 'var' inherits the dllimport attribute, and the const-init evaluation for 'p' is rejected because of the dllimport attribute. MSVC cl is fine with the snipped above. I think it's fine to accept the exemple above since the body of the function will be discarded anyway; and the inlined version of the function will contain a reference to the imported function-static symbol, like MSVC does: Full diff: https://github.com/llvm/llvm-project/pull/171628.diff 2 Files Affected:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index d81496ffd74e0..e5ca6912ed566 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2407,8 +2407,11 @@ static bool CheckLValueConstantExpression(EvalInfo &Info, SourceLocation Loc,
return false;
// A dllimport variable never acts like a constant, unless we're
- // evaluating a value for use only in name mangling.
- if (!isForManglingOnly(Kind) && Var->hasAttr<DLLImportAttr>())
+ // evaluating a value for use only in name mangling, and unless we're a
+ // static local. For the latter case, we'd still need to evaluate the
+ // constant expression in case we're inside a (inlined) function.
+ if (!isForManglingOnly(Kind) && Var->hasAttr<DLLImportAttr>() &&
+ !Var->isStaticLocal())
// FIXME: Diagnostic!
return false;
diff --git a/clang/test/SemaCXX/dllimport.cpp b/clang/test/SemaCXX/dllimport.cpp
index b7a1a62b8725b..eaf217c8e2d3b 100644
--- a/clang/test/SemaCXX/dllimport.cpp
+++ b/clang/test/SemaCXX/dllimport.cpp
@@ -1526,6 +1526,40 @@ template <typename T> struct __declspec(dllimport) PartiallySpecializedClassTemp
template <typename T> struct ExpliciallySpecializedClassTemplate {};
template <> struct __declspec(dllimport) ExpliciallySpecializedClassTemplate<int> { void f() {} };
+// Function-local static constexpr in dllimport function (or class).
+struct DLLImportFuncWithConstexprStatic {
+#if defined(GNU)
+// expected-warning@+2{{'dllimport' attribute ignored on inline function}}
+#endif
+ __declspec(dllimport) static const int *func() {
+ static constexpr int value = 42;
+ static constexpr const int *p = &value;
+ static_assert(*p == 42, "");
+ return p;
+ }
+};
+const int* (*pFunc)() = &DLLImportFuncWithConstexprStatic::func;
+bool UsedDLLImportFuncWithConstexprStatic() {
+ return pFunc() == DLLImportFuncWithConstexprStatic::func();
+}
+
+#if !defined(PS)
+struct DLLImportInlineFuncWithConstexprStatic {
+#if defined(GNU)
+ // expected-warning@+2{{'dllimport' attribute ignored on inline function}}
+#endif
+ __declspec(dllimport) __forceinline static const int* funcForceInline() {
+ static constexpr int value = 42;
+ static constexpr const int* p = &value;
+ static_assert(*p == 42, "");
+ return p;
+ }
+};
+const int* (*pFuncForceInline)() = &DLLImportInlineFuncWithConstexprStatic::funcForceInline;
+bool UsedDLLImportInlineFuncWithConstexprStatic() {
+ return pFuncForceInline() == DLLImportInlineFuncWithConstexprStatic::funcForceInline();
+}
+#endif // !PS
//===----------------------------------------------------------------------===//
// Classes with template base classes
|
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
Hmm, I haven't thought deeply about these kinds of things in a while, so I'm struggling to wrap my head around this.
I think it's fine to accept the example above since the body of the function will be discarded anyway; and the inlined version of the function will contain a reference to the imported function-static symbol, like MSVC does
Why does it matter whether the function gets inlined or not?
Despite the __forceinline I don't think it's guaranteed to get inlined, and in your change to ExprConstant.cpp, you're not checking for it anyway?
Because the behavior is not the same, however the address of the local static must remain the same in both cases:
The above was only a observation I couldn't make before, because of the compilation error (in the description above). That observation is unrelated with my change. |
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
Trying to wrap my head around this. Part of the weirdness comes from having an initializer for a dllimport variable in the first place.
We would typically not allow:
__declspec(dllimport) int x = 42;
(error: definition of dllimport data)
But we allow it when the dllimport is inherited via the function for a static local, because otherwise a lot of code would break:
__declspec(dllimport) constexpr const int* f() {
static constexpr int x = 42;
return &x;
}
We already seem okay evaluating the address as a constant expression:
__declspec(dllimport) constexpr const int* f() {
static constexpr int x = 42;
return &x;
}
static_assert(*f() == 42);
But not like this:
__declspec(dllimport) constexpr const int* f() {
static constexpr int x = 42;
static constexpr const int *p = &x;
return p;
}
static_assert(*f() == 42);
Even though it's really the same thing? I'm not sure why those two are different (that would be interesting to know!).
Maybe this is a simpler test case to work with?
In the updated comment, you add "For the [static local] case, we'd still need to evaluate the constant expression in case we're inside a (inlined) function."
But isn't there a risk that the expression can leak outside the function, and we way try to use it in some constexpr context where it doesn't actually work?
| // A dllimport variable never acts like a constant, unless we're | ||
| // evaluating a value for use only in name mangling. | ||
| if (!isForManglingOnly(Kind) && Var->hasAttr<DLLImportAttr>()) | ||
| // evaluating a value for use only in name mangling, and unless we're a |
3 participants
Consider the following:
With clang-cl, this generates an error:
MSVC cl.exe does not generate such error with the same snippet.
The problem here is that the static variable 'var' inherits the dllimport attribute, and the const-init evaluation for 'p' is rejected because of the dllimport attribute.
I think it's fine to accept the example above since the body of the function will be discarded anyway; and the inlined version of the function will contain a reference to the imported function-static symbol, like MSVC does:
Thanks to @jfmarquis for crafting a reproducer.