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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
