[cfe-commits] r133450 - in /cfe/trunk: lib/CodeGen/CodeGenModule.cpp lib/Sema/SemaDecl.cpp test/CodeGen/attr-weak-import.c
dgregor at apple.com
Wed Jun 22 14:16:53 PDT 2011
On Jun 22, 2011, at 2:16 PM, jahanian wrote:
> On Jun 22, 2011, at 1:58 PM, Douglas Gregor wrote:
>> 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
>>> 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
>>> 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
>>> 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?
> This is OK with me. In fact, as I was trying test matrix, I ran into this. It was not in original user's complaint.
More information about the cfe-commits