[cfe-commits] [PATCH] Don't eagerly emit a global static merged with a local extern.

[Douglas Gregor]

On Dec 19, 2012, at 7:14 PM, Rafael Ávila de Espíndola <rafael.espindola at gmail.com> wrote:

> When we are visiting the extern declaration of 'i' in
> 
> static int i = 99;
> int foo() {
>  extern int i;
>  return i;
> }
> 
> We should not try to handle it as if it was an function static. That is, we
> must consider the written storage class.

Yeah. The fact that we represent the "extern int i;" as having "foo" as its semantic DeclContext is a longstanding problem, and this bug falls out of that damage.

> Fixing this then exposes that the assert in EmitGlobalVarDeclLValue and the
> if leading to its call are not completely accurate. They were passing before
> because the second decl was marked as having external storage. I changed them
> to check the linkage, which I find easier to understand.

Yes, that's far better.

> Last but not least, there is something strange going on with cuda and opencl.
> My guess is that the linkage computation for these languages needs to be
> audited, but I didn't want to change that in this patch so I just updated
> the storage classes to keep the current behavior.

That's fine. Patch looks good.

	- Doug

> Thanks to Reed Kotler for reporting this.
> ---
> lib/CodeGen/CGDecl.cpp        |  2 +-
> lib/CodeGen/CGExpr.cpp        |  5 ++---
> lib/Sema/SemaDecl.cpp         |  9 +++++++--
> test/CodeGen/linkage-redecl.c | 10 +++++++++-
> 4 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/CodeGen/CGDecl.cpp b/lib/CodeGen/CGDecl.cpp
> index e01d56b..7de4637 100644
> --- a/lib/CodeGen/CGDecl.cpp
> +++ b/lib/CodeGen/CGDecl.cpp
> @@ -107,7 +107,7 @@ void CodeGenFunction::EmitDecl(const Decl &D) {
> /// EmitVarDecl - This method handles emission of any variable declaration
> /// inside a function, including static vars etc.
> void CodeGenFunction::EmitVarDecl(const VarDecl &D) {
> -  switch (D.getStorageClass()) {
> +  switch (D.getStorageClassAsWritten()) {
>   case SC_None:
>   case SC_Auto:
>   case SC_Register:
> diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp
> index daab163..0f88b3c 100644
> --- a/lib/CodeGen/CGExpr.cpp
> +++ b/lib/CodeGen/CGExpr.cpp
> @@ -1583,8 +1583,7 @@ EmitBitCastOfLValueToProperType(CodeGenFunction &CGF,
> 
> static LValue EmitGlobalVarDeclLValue(CodeGenFunction &CGF,
>                                       const Expr *E, const VarDecl *VD) {
> -  assert((VD->hasExternalStorage() || VD->isFileVarDecl()) &&
> -         "Var decl must have external storage or be a file var decl!");
> +  assert(VD->hasLinkage() && "Var decl must have linkage!");
> 
>   llvm::Value *V = CGF.CGM.GetAddrOfGlobalVar(VD);
>   llvm::Type *RealVarTy = CGF.getTypes().ConvertTypeForMem(VD->getType());
> @@ -1658,7 +1657,7 @@ LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) {
> 
>   if (const VarDecl *VD = dyn_cast<VarDecl>(ND)) {
>     // Check if this is a global variable.
> -    if (VD->hasExternalStorage() || VD->isFileVarDecl()) 
> +    if (VD->hasLinkage())
>       return EmitGlobalVarDeclLValue(*this, E, VD);
> 
>     bool isBlockVariable = VD->hasAttr<BlocksAttr>();
> diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
> index c1afaa7..9069c21 100644
> --- a/lib/Sema/SemaDecl.cpp
> +++ b/lib/Sema/SemaDecl.cpp
> @@ -4284,8 +4284,10 @@ Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC,
>   if (getLangOpts().OpenCL) {
>     // Set up the special work-group-local storage class for variables in the
>     // OpenCL __local address space.
> -    if (R.getAddressSpace() == LangAS::opencl_local)
> +    if (R.getAddressSpace() == LangAS::opencl_local) {
>       SC = SC_OpenCLWorkGroupLocal;
> +      SCAsWritten = SC_OpenCLWorkGroupLocal;
> +    }
>   }
> 
>   bool isExplicitSpecialization = false;
> @@ -4420,8 +4422,11 @@ Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC,
>     // CUDA B.2.5: "__shared__ and __constant__ variables have implied static
>     // storage [duration]."
>     if (SC == SC_None && S->getFnParent() != 0 &&
> -       (NewVD->hasAttr<CUDASharedAttr>() || NewVD->hasAttr<CUDAConstantAttr>()))
> +        (NewVD->hasAttr<CUDASharedAttr>() ||
> +         NewVD->hasAttr<CUDAConstantAttr>())) {
>       NewVD->setStorageClass(SC_Static);
> +      NewVD->setStorageClassAsWritten(SC_Static);
> +    }
>   }
> 
>   // In auto-retain/release, infer strong retension for variables of
> diff --git a/test/CodeGen/linkage-redecl.c b/test/CodeGen/linkage-redecl.c
> index 09b51f0..14112fe 100644
> --- a/test/CodeGen/linkage-redecl.c
> +++ b/test/CodeGen/linkage-redecl.c
> @@ -1,4 +1,11 @@
> -// RUN: %clang_cc1 -emit-llvm %s -o - |grep internal
> +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
> +
> +// CHECK: @test2_i = internal global i32 99
> +static int test2_i = 99;
> +int test2_f() {
> +  extern int test2_i;
> +  return test2_i;
> +}
> 
> // C99 6.2.2p3
> // PR3425
> @@ -9,3 +16,4 @@ void g0() {
> }
> 
> extern void f(int x) { } // still has internal linkage
> +// CHECK: define internal void @f
> -- 
> 1.7.11.7
>