rsmith added a comment.
We've had problems in the past with speculative emission of values like this
resulting in us producing invalid symbol references. (Two cases I remember: an
`available_externally` symbol references a symbol that should be emitted as
`linkonce_odr` but which is not emitted in this TU, and an
`available_externally` symbol references a symbol with `hidden` visibility that
is actually defined in a different DSO. In both cases, if the value of the
`available_externally` symbol is actually used, you end up with a program with
an invalid symbol reference.)
I don't immediately see any ways that this change would be susceptible to such
a problem, so perhaps our best bet is to try it and see if it actually breaks
anything.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2374
+ llvm::Constant *Init = nullptr;
+ if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr<DLLImportAttr>())
{
+ const VarDecl *InitDecl;
----------------
Applying this for only `constexpr` variables seems unnecessarily conservative;
it seems we could equally do this for any variable that is `const` and has no
`mutable` fields (though we'd need to check for `EmitConstantInit` failing in
the general case).
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2376
+ const VarDecl *InitDecl;
+ const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+ if (InitExpr) {
----------------
ahatanak wrote:
> Does getAnyInitializer ever return a null pointer here when D is a c++11
> constexpr?
Only in error cases, which shouldn't get this far.
================
Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:1
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu |
FileCheck %s
+
----------------
Please also test what happens in C++1z mode, particularly as static constexpr
data members are implicitly `inline` there.
================
Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:15
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+
----------------
mehdi_amini wrote:
> Looks like I have a bug here, this should be an internal.
I would imagine that we skip promotion of declaration to definition in this
case if we already have a definition.
To that end, please add a testcase like this:
```
struct Bar {
static constexpr int BAZ = 42;
};
auto *use = &Bar::BAZ;
const int Bar::BAZ;
```
... to make sure that we convert the definition of `Bar::BAZ` from
`available_externally` to a strong definition (I think we should end up with
`weak_odr` here).
https://reviews.llvm.org/D34992
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
