Moving QIR runtime build config into CMakeLists. #863
Conversation
swernli
left a comment
swernli
left a comment
There was a problem hiding this comment.
This looks good to me! One question below on the UBSan.ignore file and how that might be moved into the CMakeLists.txt too. But as is I think this is a good change to make!
| set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -DDEBUG") | ||
|
|
||
| if (NOT WIN32) | ||
| set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=undefined -fsanitize=float-divide-by-zero -fsanitize=unsigned-integer-overflow -fsanitize=implicit-conversion -fsanitize=local-bounds -fsanitize=nullability") |
There was a problem hiding this comment.
nitpick: Is there a reason to not just combine this and the lines below into a single long string with newlines? Something like:
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} \
-fsanitize=undefined \
-fsanitize=float-divide-by-zero \
...
")
src/Qir/qir-utils.ps1
Outdated
| # https://releases.llvm.org/11.0.1/tools/clang/docs/SanitizerSpecialCaseList.html | ||
| $sanitizeFlags = " -fsanitize-blacklist=" # https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#suppressing-errors-in-recompiled-code-ignorelist | ||
| # https://releases.llvm.org/11.0.1/tools/clang/docs/SanitizerSpecialCaseList.html | ||
| $sanitizeFlags += (Join-Path $Path .. UBSan.ignore) |
There was a problem hiding this comment.
Is not having this flag in the CMakeLists.txt going to interfere with the sanitizer behavior when compiling directly through there? Would it be worth putting this path into a define variable that is then used in the CMakeLists.txt?
| if (($IsMacOS) -or ((Test-Path Env:/AGENT_OS) -and ($Env:AGENT_OS.StartsWith("Darwin")))) | ||
| { | ||
| if (($IsMacOS) -or ((Test-Path Env:/AGENT_OS) -and ($Env:AGENT_OS.StartsWith("Darwin")))) { |
There was a problem hiding this comment.
I would recommend to do the unrelated changes like this one in a separate small preparatory PR.
The current PR has certain logic relocation. And it takes some time to double-check that the correctness of the logic is preserved. The multiple small unrelated changes like this one complicate the analysis of the correctness.
Is also applicable to the lines that change the number of spaces.
kuzminrobin
left a comment
kuzminrobin
left a comment
There was a problem hiding this comment.
I'm done with the CR pass.
I would recommend to preserve the sanitizer comments. Other notes are up to you.
|
@kuzminrobin @swernli Thanks for taking a look. I left the flags separate rather than consolidate as I was unsure as to which way to go with the comments. It sounds like the preferred path is to migrate the comments into the |
kuzminrobin
left a comment
kuzminrobin
left a comment
There was a problem hiding this comment.
LGTM. Approving.
I would consider handling WARNING_FLAGS before the SANITIZE_FLAGS.
Also the fragment
# Always use available Spectre mitigations where available
if (NOT APPLE)
set(WARNING_FLAGS "${WARNING_FLAGS} -mspeculative-load-hardening -mretpoline")
endif()does not seem applicable to warnings. So you might want to use a different var instead of WARNING_FLAGS.
Up to you.
…igations variable associated with warnings.
|
LGTM, re-approving. |
|
@kuzminrobin Nice catch on the Spectre mitigations. Pushed what you requested. |
* Moving QIR runtime build config into qir_cmake_include CMakeLists.
* Use explicit static VC runtime on Windows (#851) * Use explicit static VC runtime on Windows This change updates the QIR runtime to use an explicit path to the Spectre-mitigated, static VC runtime on Windows. Using a full path here overrides the selection logic and ensures the chosen libcmt[d] and vcruntime[d] are loaded. * Add "-sort" flag * Include version number in find pattern * Include libcpmt library as well * Use spectre component name * Append AppId to UserAgent when calling Azure Quantum APIs (#858) * Add `CX`, `CY` and `CZ` to Type1, Type3 target packages. (#842) (#861) * Moving QIR runtime build config into CMakeLists. (#863) * Moving QIR runtime build config into qir_cmake_include CMakeLists. Co-authored-by: Stefan J. Wernli <[email protected]> Co-authored-by: XField <[email protected]> Co-authored-by: Ian Davis <[email protected]>
To aid in consumption of the QIR Runtime, most configuration has been moved into the
CMakeLists.txt.