The linkage changes in r359260 break MIDL code

[llvmbot]

Bugzilla Link	41817
Version	trunk
OS	Windows NT
Blocks	llvm/llvm-project#42705
Reporter	LLVM Bugzilla Contributor
CC	@zmodem,@slacka,@nico,@zygoloid,@rnk,@sylvestre,@tstellar

Extended Description

The following code builds successfully with r359259 but not r359260:

$ cat a.cpp extern const int x; static const int x = 3; const int* foo() { return &x; }

$ cat b.cpp extern const int x; static const int x = 7; const int* bar() { return &x; }

$ cat c.cpp const int foo(); const int bar(); int main(int argc, char*) { return argc ? foo() : *bar(); }

$ clang-cl -c a.cpp b.cpp c.cpp

$ lld-link -nodefaultlib -entry:main a.obj b.obj c.obj lld-link: error: duplicate symbol: int const x in a.obj and in b.obj

This pattern can be seen in code generated by MIDL, e.g. https://searchfox.org/mozilla-central/search?q=text%3AHandlerData__MIDL_ProcFormatString

Since there are no templates involved here, I'm guessing this change in behavior was unintended?

[nico]

mentioned in issue llvm/llvm-project#42705

[rnk]

mentioned in issue llvm/llvm-bugzilla-archive#42011

[zmodem]

It doesn't sound like there's any traction here. I don't think we can block the 9 release on this, but hopefully it can get fixed for 9.0.1.

[llvmbot]

Nominating this as a 9.0.0 blocker since it's a regression from the previous release.

[llvmbot]

The link in comment 3 doesn't work, the URL for the Chromium reproducer should be: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190513/271324.html (htm -> html)

[rnk]

I'd say it's fallen off of the end of my priority list, unfortunately.

I think the agreed upon behavior is that we:

• keep warning as we do
• find a way to avoid the assert by clearing any cached linkage
• forcibly update the cached linkage with the new linkage (internal/static)
• add some new callback from Sema to CodeGen to allow downgrading the global linkage from external to internal

All of those change are only when compiling C code, as in MIDL generated code. MSVC leaves const globals with extern declarations as external when compiling C++, which is clang's new behavior, so we should just warn and drop the static storage class specifier like we do today.

[llvmbot]

rnk, just checking, is this still on your radar?

[rnk]

I chatted with Richard about this, and I think we probably want to try to implement this compatibility hack a bit more intentionally, i.e. actually take some action when we encounter these things.

Someone must have added this compatibility hack earlier on in the project history, and simply downgraded the error into a warning, which doesn't actually implement MSVC's behavior. The fix is to finish the job.

[llvmbot]

Mostly unrelated, but: dmajor, do you actually need the _c.c files?

I gave it a try, and it turns out that we do need them. Interesting tip though; thanks.

[nico]

Mostly unrelated, but: dmajor, do you actually need the _c.c files? If you pass /client none to midl.exe, it looks like they aren't generated. Wd don't have the _c.c files as part of the chromium build, which is why we didn't notice that. (We should still fix this bug of course.)

[zmodem]

Should be fixed in r360637.

I had to revert that in r360657 because it broke Chromium, see http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190513/271324.htm

[ec04fc15-fa35-46f2-80e1-5d271f2ef708]

Should be fixed in r360637.

[ec04fc15-fa35-46f2-80e1-5d271f2ef708]

Yes, this was unintended. (The code is invalid, but we support it under -fms-compatibility. Someone should probably file a bug on MIDL to get it to stop generating ill-formed code.)

[llvmbot]

assigned to @rnk