The linkage changes in r359260 break MIDL code

[Quuxplusone]

Bugzilla Link	PR41817
Status	REOPENED
Importance	P normal
Reported by	dmajor (dmajor@bugmail.cc)
Reported on	2019-05-09 07:36:19 -0700
Last modified on	2019-11-27 09:23:46 -0800
Version	trunk
Hardware	PC Windows NT
CC	hans@chromium.org, htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, lukebenes@hotmail.com, neeilans@live.com, nicolasweber@gmx.de, richard-llvm@metafoo.co.uk, rnk@google.com, sylvestre@debian.org, tstellar@redhat.com
Fixed by commit(s)
Attachments
Blocks	PR43360
Blocked by
See also

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?

[Quuxplusone]

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.)

[Quuxplusone]

Should be fixed in r360637.

[Quuxplusone]

(In reply to Richard Smith from comment #2)
> 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

[Quuxplusone]

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.)

[Quuxplusone]

(In reply to Nico Weber from comment #4)
> 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.

[Quuxplusone]

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.

[Quuxplusone]

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

[Quuxplusone]

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.

[Quuxplusone]

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)

[Quuxplusone]

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

[Quuxplusone]

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.