Page MenuHomePhabricator

[libunwind] Enable the SEH unwinding codepaths if building in MSVC mode
Needs ReviewPublic

Authored by mstorsjo on Aug 26 2020, 4:39 AM.

Details

Reviewers
compnerd
Group Reviewers
Restricted Project
Summary

The __SEH__ built-in define is only defined in mingw mode; in MSVC mode it's implied.

This makes a libunwind built with clang-cl in MSVC mode actually do unwinding using the SEH information instead of DWARF.

After D86041, this should make the now successfully built libunwind actually do the right thing as well.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 26 2020, 4:39 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 26 2020, 4:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Aug 26 2020, 4:39 AM

I don't think that this is entirely true. I believe it is possible to disable SEH with cl, so I think that it is better to conditionalize it as if defined(__SEH__) || defined(_SEH) since IIRC, _SEH is the define that cl uses.

I don't think that this is entirely true. I believe it is possible to disable SEH with cl, so I think that it is better to conditionalize it as if defined(__SEH__) || defined(_SEH) since IIRC, _SEH is the define that cl uses.

Hmm, I don't see it being defined with cl.exe as far as I can see, and in any case, clang-cl doesn't define it either.

@compnerd - Any followup on this one?

It is definitely possible to disable SEH via /EHs- though

It is definitely possible to disable SEH via /EHs- though

Sure, but that's not visible via preprocessor macros as far as I know. (It's not mentioned in https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros, MSVC doesn't support -E -dM, and this option doesn't change clang-cl's predefined macros.)

And regardless - if building libunwind for an MSVC environment, the SEH backend (as opposed to the dwarf or SJLJ ones) is the one that makes most sense to enable by default, as far as I can see. I guess you'd theoretically want to be able to enable the dwarf backend as well, but that should probably require more explicit opt-in.

I was thinking more the SjLj backend. I am okay with an additional option, but this currently would force enable the SEH backend.