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

fix Issue 24129 - ImportC: MS-Link cannot handle multiple COMDATs wit… #15585

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

WalterBright
Copy link
Member

…h the same name

Took me a while to figure out where to put this tweak. Although it is meant just for MS-LINK, I enabled it for all targets to flush out any bugs in it.

@WalterBright WalterBright added the ImportC Pertaining to ImportC support label Sep 8, 2023
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
24129 critical ImportC: MS-Link cannot handle multiple COMDATs with the same name

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15585"

fd._linkage == LINK.c &&
!fd.skipCodegen) // code gen is desired
{
__gshared DsymbolTable Csymtab; // sorry about another global variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

img

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have to be declared inline? The rest of the compiler has a more (...) global pattern for the global variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WalterBright can we make Csymtab a member variable of the ToSymbol visitor class? If not, why? Perhaps we can move it up the call stack and pass it as a parameter to the visitor constructor, in case there are multiple instances?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to prove that this fix works. Refactoring it then can (and should) be an independent task.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Sep 8, 2023

Should we really be fixing linker issues in the compiler?

@WalterBright
Copy link
Member Author

Should we really be fixing linker issues in the compiler?

The main reason DMD (and DMC++) had its own linker was it was easier than concocting endless workarounds for MS-Link bugs.

We can't fix the linker! So what else can we do?

@WalterBright
Copy link
Member Author

Why is this not approved?

@RazvanN7
Copy link
Contributor

There's this comment from @PetarKirov which remains unaddressed.

@WalterBright
Copy link
Member Author

@RazvanN7 it's addressed now.

@WalterBright WalterBright merged commit 67fc169 into dlang:master Sep 20, 2023
40 of 43 checks passed
@WalterBright WalterBright deleted the fix24129 branch September 20, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix ImportC Pertaining to ImportC support
Projects
None yet
5 participants