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

Revert result macros to be noexcept again #246

Merged
merged 3 commits into from
Aug 31, 2022
Merged

Revert result macros to be noexcept again #246

merged 3 commits into from
Aug 31, 2022

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jun 11, 2022

A number of result macros aren't declared as noexcept which causes C26447
warnings if they're used within a noexcept context. This can make them
uncomfortable to use in the Terminal project which has this warning enabled.

It appears as if the noexcept attribute was removed in/around OS PR !2943876,
when C++17 was enabled for a project. This resulted in a conflict with the
__forceinline attribute and caused the FailFast_IfWin32Error function
to be exported from the .lib erroneously. DevDiv-748887 seems to
be the related bug report for MSVC and was resolved in VS 16.1.

This PR was tested by building (chk, dev) the .lib mentioned in !2943876
and comparing the dumpbin /SYMBOLS output before and after the change.
Apart from a size reduction of 16 bytes, nothing changed.

@jonwis jonwis requested review from dunhor and DefaultRyan June 13, 2022 18:49
Copy link
Member

@jonwis jonwis left a comment

Choose a reason for hiding this comment

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

This might be a tricky one to bring back into the OS, depending on how complicated (or not) people's logging/telemetry callouts are. Overall goodness, though. Thanks.

@lhecker
Copy link
Member Author

lhecker commented Aug 23, 2022

@dunhor @DefaultRyan Could you help me get this PR reviewed? 🙂

@dunhor
Copy link
Member

dunhor commented Aug 31, 2022

This might be a tricky one to bring back into the OS, depending on how complicated (or not) people's logging/telemetry callouts are. Overall goodness, though. Thanks.

Ideally changes like this should go into the OS directly for the obvious reasons, however I'm not opposed to bringing it in through here. There's just a non-zero risk that it will need to be reverted if issues arise. It is the responsibility of the author to do due diligence, but it sounds like that's been done here so I have no objections.

@dunhor dunhor merged commit d84d4e6 into microsoft:master Aug 31, 2022
@lhecker lhecker deleted the user/lhecker/result-macros-noexcept branch August 31, 2022 20:57
dunhor added a commit that referenced this pull request Sep 10, 2022
@dunhor
Copy link
Member

dunhor commented Sep 10, 2022

FYI - this has been reverted. The relevant error is:

error C4981: Warbird: function 'long __stdcall wil::details::in1diag3::Log_IfFailed(void *,unsigned int,char const *,long)' marked as __forceinline not inlined because it contains exception semantics

The (maybe) good news is that the impact didn't seem too broad, so maybe there's hope in fixing them up. Either way, this change likely needs to be done in the OS repo. I tried looking into the cause, but I was ultimately unable to repro it locally and kind of ran out of time for investigation.

@lhecker
Copy link
Member Author

lhecker commented Sep 10, 2022

Ugh, damn. I'm extremely sorry for the extra work I caused for you. 😔
I'll try to submit an internal PR for this sometime soon.

@dunhor
Copy link
Member

dunhor commented Sep 13, 2022

No worries. Like I said, the impact was very limited. IIRC it was like 2 directories (albeit in the OneCore build, so IDK if the build just didn't make it far enough to expose other failures.

Looking back now, it seems 'C4981' is just a warning, so it may very well be the case that the warning can be avoided with a localized disabling of the warning. On top of being an issue with an obscure compiler feature, if it didn't take me so long to try and get a repro - and had I actually succeeded in getting a repro - I probably could have just tried it out myself, but I ultimately ran out of time :/

dunhor added a commit that referenced this pull request Sep 14, 2022
* Sync with OS commit 7304518c5f69f8c5d9d05ed36cc79c7cb0396b5e

* Build fixes

* Unreachable code warnings

* Fixes to get kernel code building again

* Revert "Revert result macros to be noexcept again (#246)"

This reverts commit d84d4e6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants