<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><strong>From: </strong>Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org">hans@chromium.org</a>></span><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr"><strong>Date: </strong>Wed, May 15, 2019 at 1:22 AM<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, May 14, 2019 at 6:35 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> 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)?<br>
<br>
Maybe rnk has opinions too, but if we can get away with it, I think it<br>
would be nice if we could not claim to support this, maybe not error<br>
but dropping the static redeclaration of 'a' above with a warning. We<br>
could fix Chromium's code, maybe we can poke MS to fix the midl<br>
generated code, and maybe Firefox and others can work around the<br>
duplicate symbol error in PR41871 by passing "/client none" to<br>
midl.exe in the meantime.<br>
<br>
For my reduction above, I think we used to:<br>
- in clang 8, emit a with internal linkage (getting it right through<br>
luck somehow?)<br>
- after r359259, emit it with external linkage (causing PR41871,<br>
breaking Firefox)<br>
- after r360637, assert<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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. =/</div><div><br></div><div>$ cat t.cpp</div><div>typedef struct Foo { int x,y; } Foo;</div><div>extern const Foo my_global;</div><div>static const Foo my_global = { 1, 2 };</div><div><br></div><div>$ cl -nologo -TC -c t.cpp && dumpbin -symbols t.obj | grep my_global</div><div>t.cpp</div><div>008 00000000 SECT3  notype       Static       | my_global</div><div><br></div><div>$ cl -nologo -TP -c t.cpp && dumpbin -symbols t.obj | grep my_global</div><div>t.cpp</div><div>008 00000000 SECT3  notype       External     | ?my_global@@3UFoo@@B (struct Foo const my_global) </div><div><br></div><div>So, I think in C++ mode, MSVC just ignores the second static.</div><div><br></div><div>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?</div><div><br></div><div>Those are the best ideas I have so far. :)</div></div></div></div></div></div>