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

Add option to enable spectre mitigation in MSVC #1125

Closed

Conversation

donny-dont
Copy link
Contributor

Adds an option ENABLE_MSVC_SPECTRE_MITIGATION which will add the /Qspectre flag on MSVC.

@donny-dont
Copy link
Contributor Author

donny-dont commented Nov 13, 2024

This is to align the build with the one in vcpkg https://github.com/microsoft/vcpkg/blob/e109e2de620e50fa521f52748c47857ad2fbe350/ports/libressl/0002-suppress-msvc-warnings.patch#L9-L12

If its a no that's fine will keep the patch locally there and close this. Just throwing it out there since vcpkg is building it in this manner.

@botovq
Copy link
Contributor

botovq commented Nov 13, 2024

This makes sense to me. I'll leave this open for a bit to see if anyone using windows builds objects. @vszakats, any opinion on this?

@vszakats
Copy link
Contributor

This can be done by passing -DCMAKE_C_FLAGS=/Qspectre to CMake.

A dedicated, nearly 1 to 1 MSVC-specific CMake option doesn't simplify this IMO. Perhaps it would, if it enabled it for other compilers. But, there are several spectre mitigations (e.g. in gcc), making it somewhat difficult to figure out a single combination that makes everyone happy. Dropping "MSVC" from the option name/desc would be a nice touch either way.

@donny-dont
Copy link
Contributor Author

This can be done by passing -DCMAKE_C_FLAGS=/Qspectre to CMake.

A dedicated, nearly 1 to 1 MSVC-specific CMake option doesn't simplify this IMO. Perhaps it would, if it enabled it for other compilers. But, there are several spectre mitigations (e.g. in gcc), making it somewhat difficult to figure out a single combination that makes everyone happy. Dropping "MSVC" from the option name/desc would be a nice touch either way.

Sounds good. Any particular options you want for this on clang/gcc? Researching it some on my end as well.

Adds an option `ENABLE_MSVC_SPECTRE_MITIGATION` which will add the `/Qspectre` flag on MSVC.
@vszakats
Copy link
Contributor

These are the ones I'm aware of, it's a 2021 list, there may be more options since:

  • llvm/clang: -mretpoline (since 6.0.0, perhaps also in 5.0.2)
  • gcc: -mindirect-branch -mfunction-return -mindirect-branch-register

IIRC they caused a significant performance drop, so ended up not using them.

Also for MSVC there is: /Qspectre-load, /Qspectre-load-cf, /Qspectre-jmp.

@donny-dont
Copy link
Contributor Author

This can be done by passing -DCMAKE_C_FLAGS=/Qspectre to CMake.

After talking to the vcpkg folks they would also prefer this way of doing things. Since there are so many options and trade offs I think thats the path so I will close this.

Thanks again for your time on this!

@donny-dont donny-dont closed this Nov 14, 2024
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.

3 participants