r360637 - PR41817: Fix regression in r359260 that caused the MS compatibility

[Reid Kleckner via cfe-commits]

*From: *Hans Wennborg <hans at chromium.org>
*Date: *Wed, May 15, 2019 at 1:22 AM

> On Tue, May 14, 2019 at 6:35 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
> > 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
>

The files are generated by the most up to date MIDL compiler, 8.0.1, and I
think it would be good to maintain compatibility with it until it a bug fix
is released.

It looks like MSVC gives an extern global turned-static static linkage in C
mode, and external linkage in C++ mode. It doesn't even agree with itself.
=/

$ cat t.cpp
typedef struct Foo { int x,y; } Foo;
extern const Foo my_global;
static const Foo my_global = { 1, 2 };

$ cl -nologo -TC -c t.cpp && dumpbin -symbols t.obj | grep my_global
t.cpp
008 00000000 SECT3  notype       Static       | my_global

$ cl -nologo -TP -c t.cpp && dumpbin -symbols t.obj | grep my_global
t.cpp
008 00000000 SECT3  notype       External     | ?my_global@@3UFoo@@B
(struct Foo const my_global)

So, I think in C++ mode, MSVC just ignores the second static.

We could probably re-land Richard's change (although it's quite a lot of
code...), but... drop the conflicting `static` on the floor
if hasLinkageBeenComputed() returns true. That would work around the
assert, right?

Those are the best ideas I have so far. :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190515/229299ea/attachment.html>