On Tue, May 14, 2019 at 6:35 PM Richard Smith <rich...@metafoo.co.uk> wrote: > > On Tue, 14 May 2019, 03:09 Hans Wennborg via cfe-commits, > <cfe-commits@lists.llvm.org> wrote: >> >> Actually, we didn't notice r359260 in Chromium, however this one >> (r360637) caused Clang to start asserting during our builds >> (https://bugs.chromium.org/p/chromium/issues/detail?id=962840), here's >> a reduced test case: >> >> -- >> extern int a; >> static int *use1 = &a; >> int **use2 = &use1; >> static int a = 0; >> -- >> >> $ clang.bad -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-obj >> -fms-extensions /tmp/a.cc >> clang.bad: /work/llvm.monorepo/clang/lib/AST/Decl.cpp:1494: >> clang::LinkageInfo clang::LinkageComputer::getLVForDecl(const >> clang::NamedDecl*, clang::LVComputationKind): Assertion >> `D->getCachedLinkage() == LV.getLinkage()' failed. >> >> >> I've reverted this one in r360657 in the meantime. > > > Yep, I'm not at all surprised. Perhaps we should stop claiming to support > this extension, given that it fundamentally violates assumptions made by > clang (that linkage doesn't change after the first declaration). Presumably > we instead used to miscompile the example you give above (and after the > revert, miscompile it again now)?
Maybe rnk has opinions too, but if we can get away with it, I think it would be nice if we could not claim to support this, maybe not error but dropping the static redeclaration of 'a' above with a warning. We could fix Chromium's code, maybe we can poke MS to fix the midl generated code, and maybe Firefox and others can work around the duplicate symbol error in PR41871 by passing "/client none" to midl.exe in the meantime. For my reduction above, I think we used to: - in clang 8, emit a with internal linkage (getting it right through luck somehow?) - after r359259, emit it with external linkage (causing PR41871, breaking Firefox) - after r360637, assert >> From: Richard Smith via cfe-commits <cfe-commits@lists.llvm.org> >> Date: Tue, May 14, 2019 at 2:24 AM >> To: <cfe-commits@lists.llvm.org> >> >> > Author: rsmith >> > Date: Mon May 13 17:27:16 2019 >> > New Revision: 360637 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=360637&view=rev >> > Log: >> > PR41817: Fix regression in r359260 that caused the MS compatibility >> > extension allowing a "static" declaration to follow an "extern" >> > declaration to stop working. >> > >> > Added: >> > cfe/trunk/test/CodeGen/ms-compat-extern-static.c >> > Modified: >> > cfe/trunk/lib/AST/Decl.cpp >> > >> > Modified: cfe/trunk/lib/AST/Decl.cpp >> > URL: >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=360637&r1=360636&r2=360637&view=diff >> > ============================================================================== >> > --- cfe/trunk/lib/AST/Decl.cpp (original) >> > +++ cfe/trunk/lib/AST/Decl.cpp Mon May 13 17:27:16 2019 >> > @@ -613,12 +613,41 @@ static LinkageInfo getExternalLinkageFor >> > static StorageClass getStorageClass(const Decl *D) { >> > if (auto *TD = dyn_cast<TemplateDecl>(D)) >> > D = TD->getTemplatedDecl(); >> > - if (D) { >> > - if (auto *VD = dyn_cast<VarDecl>(D)) >> > - return VD->getStorageClass(); >> > - if (auto *FD = dyn_cast<FunctionDecl>(D)) >> > - return FD->getStorageClass(); >> > + if (!D) >> > + return SC_None; >> > + >> > + if (auto *VD = dyn_cast<VarDecl>(D)) { >> > + // Generally, the storage class is determined by the first >> > declaration. >> > + auto SC = VD->getCanonicalDecl()->getStorageClass(); >> > + >> > + // ... except that MSVC permits an 'extern' declaration to be >> > redeclared >> > + // 'static' as an extension. >> > + if (SC == SC_Extern) { >> > + for (auto *Redecl : VD->redecls()) { >> > + if (Redecl->getStorageClass() == SC_Static) >> > + return SC_Static; >> > + if (Redecl->getStorageClass() != SC_Extern && >> > + !Redecl->isLocalExternDecl() && >> > !Redecl->getFriendObjectKind()) >> > + break; >> > + } >> > + } >> > + return SC; >> > } >> > + >> > + if (auto *FD = dyn_cast<FunctionDecl>(D)) { >> > + auto SC = FD->getCanonicalDecl()->getStorageClass(); >> > + if (SC == SC_Extern) { >> > + for (auto *Redecl : FD->redecls()) { >> > + if (Redecl->getStorageClass() == SC_Static) >> > + return SC_Static; >> > + if (Redecl->getStorageClass() != SC_Extern && >> > + !Redecl->isLocalExternDecl() && >> > !Redecl->getFriendObjectKind()) >> > + break; >> > + } >> > + } >> > + return SC; >> > + } >> > + >> > return SC_None; >> > } >> > >> > @@ -634,7 +663,7 @@ LinkageComputer::getLVForNamespaceScopeD >> > // A name having namespace scope (3.3.6) has internal linkage if it >> > // is the name of >> > >> > - if (getStorageClass(D->getCanonicalDecl()) == SC_Static) { >> > + if (getStorageClass(D) == SC_Static) { >> > // - a variable, variable template, function, or function template >> > // that is explicitly declared static; or >> > // (This bullet corresponds to C99 6.2.2p3.) >> > >> > Added: cfe/trunk/test/CodeGen/ms-compat-extern-static.c >> > URL: >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-compat-extern-static.c?rev=360637&view=auto >> > ============================================================================== >> > --- cfe/trunk/test/CodeGen/ms-compat-extern-static.c (added) >> > +++ cfe/trunk/test/CodeGen/ms-compat-extern-static.c Mon May 13 17:27:16 >> > 2019 >> > @@ -0,0 +1,11 @@ >> > +// RUN: %clang_cc1 -emit-llvm %s -o - -fms-extensions -triple >> > x86_64-windows | FileCheck %s >> > + >> > +// CHECK: @n = internal global i32 1 >> > +extern int n; >> > +static int n = 1; >> > +int *use = &n; >> > + >> > +// CHECK: define internal void @f( >> > +extern void f(); >> > +static void f() {} >> > +void g() { return f(); } >> > >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > cfe-commits@lists.llvm.org >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
