Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The linkage changes in r359260 break MIDL code #41162

Open
llvmbot opened this issue May 9, 2019 · 14 comments
Open

The linkage changes in r359260 break MIDL code #41162

llvmbot opened this issue May 9, 2019 · 14 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla clang Clang issues not falling into any other category

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented May 9, 2019

Bugzilla Link 41817
Version trunk
OS Windows NT
Blocks #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?

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 9, 2019

assigned to @rnk

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented May 9, 2019

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

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented May 14, 2019

Should be fixed in r360637.

@zmodem
Copy link
Collaborator

zmodem commented May 14, 2019

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

@nico
Copy link
Contributor

nico commented May 14, 2019

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

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 15, 2019

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.

@rnk
Copy link
Collaborator

rnk commented May 20, 2019

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
Copy link
Collaborator Author

llvmbot commented Jun 4, 2019

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

@rnk
Copy link
Collaborator

rnk commented Jun 4, 2019

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
Copy link
Collaborator Author

llvmbot commented Jul 31, 2019

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)

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 12, 2019

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

@zmodem
Copy link
Collaborator

zmodem commented Aug 27, 2019

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.

@rnk
Copy link
Collaborator

rnk commented Nov 27, 2021

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

@nico
Copy link
Contributor

nico commented Nov 27, 2021

mentioned in issue #42705

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang Clang issues not falling into any other category
Projects
None yet
Development

No branches or pull requests

4 participants