r360637 - PR41817: Fix regression in r359260 that caused the MS compatibility
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Wed May 15 01:21:47 PDT 2019
On Tue, May 14, 2019 at 6:35 PM Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Tue, 14 May 2019, 03:09 Hans Wennborg via cfe-commits, <cfe-commits at 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 at lists.llvm.org>
>> Date: Tue, May 14, 2019 at 2:24 AM
>> To: <cfe-commits at 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 at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list