[cfe-commits] r133450 - in /cfe/trunk: lib/CodeGen/CodeGenModule.cpp lib/Sema/SemaDecl.cpp test/CodeGen/attr-weak-import.c

[Douglas Gregor]

On Jun 20, 2011, at 10:50 AM, Fariborz Jahanian wrote:

> Author: fjahanian
> Date: Mon Jun 20 12:50:03 2011
> New Revision: 133450
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=133450&view=rev
> Log:
> llvm-gcc treats a tentative definition with a previous
> (or follow up) extern declaration with weak_import as 
> an actual definition. make clang follows this behavior. 
> // rdar://9538608
> llvm-gcc treats an extern declaration with weak_import
> 
> Added:
>    cfe/trunk/test/CodeGen/attr-weak-import.c
> Modified:
>    cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>    cfe/trunk/lib/Sema/SemaDecl.cpp
> 
> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=133450&r1=133449&r2=133450&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Mon Jun 20 12:50:03 2011
> @@ -1351,7 +1351,8 @@
>            ((!CodeGenOpts.NoCommon && !D->getAttr<NoCommonAttr>()) ||
>              D->getAttr<CommonAttr>()) &&
>            !D->hasExternalStorage() && !D->getInit() &&
> -           !D->getAttr<SectionAttr>() && !D->isThreadSpecified()) {
> +           !D->getAttr<SectionAttr>() && !D->isThreadSpecified() &&
> +           !D->getAttr<WeakImportAttr>()) {
>     // Thread local vars aren't considered common linkage.
>     return llvm::GlobalVariable::CommonLinkage;
>   }
> 
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=133450&r1=133449&r2=133450&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Mon Jun 20 12:50:03 2011
> @@ -2039,6 +2039,12 @@
>   }
> 
>   mergeDeclAttributes(New, Old, Context);
> +  // weak_import on current declaration is applied to previous
> +  // tentative definiton.
> +  if (New->getAttr<WeakImportAttr>() &&
> +      Old->getStorageClass() == SC_None &&
> +      !Old->getAttr<WeakImportAttr>())
> +    Old->addAttr(::new (Context) WeakImportAttr(SourceLocation(), Context));
> 
>   // Merge the types.
>   MergeVarDeclTypes(New, Old);

This is really unfortunate, because it's intended that a declaration's linkage is an invariant in the AST. We don't depend on weak vs. non-weak so much in semantic analysis, so it's not as damaging as changing internal vs. external linkage would be, but it still causes weird breakage. For example, note how lib/AST/ExprConstant.cpp's EvalPointerValueAsBool checks for weak imports: this routine will now return different results in different parts of the translation unit, if we end up trying to evaluate the address of a variable after the first declaration and then again after it's had weak_import attribute added to it.

I know we're mimicking GCC behavior here, but I think this is a GCC bug that we should not emulate. Instead, we should complain (via a warning) that an already-declared variable cannot be made a weak_import in a subsequent declaration, and drop the WeakImportAttr from the later declaration. The motivating Radar refers to "an obscure linker test", so it's unlikely that any real code will be affected (and the warning makes sure that there's no silent breakage). 

What do you think?

	- Doug