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 10664 - Win64: exception handling does not work with COMDAT… #11736

Merged
merged 1 commit into from Sep 15, 2020

Conversation

WalterBright
Copy link
Member

… folding

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
10664 normal Win64: exception handling does not work with COMDAT folding

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#11736"

@dlang-bot dlang-bot merged commit 5ca705a into dlang:master Sep 15, 2020
MoonlightSentinel added a commit to MoonlightSentinel/dmd that referenced this pull request Sep 15, 2020
The issue related to COMDAT folding was fixed in dlang#11736.
@rainers
Copy link
Member

rainers commented Sep 15, 2020

Thanks for tackling this. While it might work for the given case, I'm not sure this is the proper fix. Can't the same issue happen with stack unwinding?

AFAICT this adds an unused symbol reference to the catch clause. I'd rather expect that the exception handling data "associated" with the function has to be compared aswell for functions to be merged. I suspect there is a weakrer coupling emitted by dmd than by the C++ compiler.

If this is considered good enough, please also remove the linker option from ini/.../sc.ini and the phobos win64.mak.

@WalterBright WalterBright deleted the fix10664 branch September 15, 2020 15:24
@WalterBright
Copy link
Member Author

While it might work for the given case, I'm not sure this is the proper fix.

I don't know about proper, but this should fix it completely.

Can't the same issue happen with stack unwinding?

Not sure what you mean. The associated EH handling data is not in a COMDAT. This means that identical handling data sections will not be merged, which is suboptimal, but it'll still work.

AFAICT this adds an unused symbol reference to the catch clause.

Yes.

I'd rather expect that the exception handling data "associated" with the function has to be compared aswell for functions to be merged.

There's no link from the function to its associated EH data.

I suspect there is a weakrer coupling emitted by dmd than by the C++ compiler.

The Win64 EH handling method here is my own invention, as at the time there was no information on how VC++ did it and I simply gave up on trying to figure it out. Perhaps there is now, but I haven't looked.

please also remove the linker option from ini/.../sc.ini and the phobos win64.mak.

OK

@rainers
Copy link
Member

rainers commented Sep 16, 2020

Not sure what you mean. The associated EH handling data is not in a COMDAT. This means that identical handling data sections will not be merged, which is suboptimal, but it'll still work.

Ah, I thought it would be in one of the .pdata or .xdata sections, but on second look it seems to be in the common ._deh$B data section. Ok, makes sense that there is no way to distinguish the functions without additional data. This stops the linker from eliminating unused functions, though.
So adding an "associated" COMDAT similar to .pdata, but with the eh data might be a possible approach.

The Win64 EH handling method here is my own invention, as at the time there was no information on how VC++ did it and I simply gave up on trying to figure it out. Perhaps there is now, but I haven't looked.

LLVM should now be a good source of information as they claim to be compatible with MSVC.

MoonlightSentinel added a commit to MoonlightSentinel/dmd that referenced this pull request Sep 17, 2020
The issue related to COMDAT folding was fixed in dlang#11736.
dlang-bot pushed a commit that referenced this pull request Sep 17, 2020
The issue related to COMDAT folding was fixed in #11736.
UplinkCoder pushed a commit to UplinkCoder/dmd that referenced this pull request Jan 5, 2021
The issue related to COMDAT folding was fixed in dlang#11736.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants