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

[Richard Smith via cfe-commits]

On Wed, 15 May 2019 at 15:54, Reid Kleckner via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> *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?
>

Yes, that seems like it could work, though I really don't like our
generated code depending on when we happen to compute linkage. In the MIDL
case, are there any uses of the global between the 'extern' declaration and
the 'static' declaration?


> Those are the best ideas I have so far. :)
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190515/600bfba3/attachment.html>