-
Notifications
You must be signed in to change notification settings - Fork 90
Re-trying the PR #881: Migrating native sim from GCC to Clang. #889
Changes from all commits
c68790a
d169bde
4b4818c
4d8962c
a78ed4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,25 +10,62 @@ if (-not (Test-Path $nativeBuild)) { | |
| } | ||
| Push-Location $nativeBuild | ||
|
|
||
| # Search for "sanitize" in | ||
| # https://clang.llvm.org/docs/ClangCommandLineReference.html | ||
| # https://man7.org/linux/man-pages/man1/gcc.1.html | ||
| $SANITIZE_FLAGS=` | ||
| "-fsanitize=undefined " ` | ||
| + "-fsanitize=shift -fsanitize=shift-base " ` | ||
| + "-fsanitize=integer-divide-by-zero -fsanitize=float-divide-by-zero " ` | ||
| + "-fsanitize=unreachable " ` | ||
| + "-fsanitize=vla-bound -fsanitize=null -fsanitize=return " ` | ||
| + "-fsanitize=signed-integer-overflow -fsanitize=bounds -fsanitize=alignment -fsanitize=object-size " ` | ||
| + "-fsanitize=float-cast-overflow -fsanitize=nonnull-attribute -fsanitize=returns-nonnull-attribute -fsanitize=bool -fsanitize=enum " ` | ||
| + "-fsanitize=vptr -fsanitize=pointer-overflow -fsanitize=builtin " ` | ||
| + "-fsanitize=implicit-conversion -fsanitize=local-bounds -fsanitize=nullability " ` | ||
| ` | ||
| + "-fsanitize=address " ` | ||
| + "-fsanitize=pointer-compare -fsanitize=pointer-subtract " ` | ||
| + "-fsanitize-address-use-after-scope " ` | ||
| + "-fno-omit-frame-pointer -fno-optimize-sibling-calls" | ||
|
|
||
| #+ "-fsanitize=unsigned-integer-overflow " # TODO(rokuzmin, #884): Disable this option for _specific_ lines | ||
| # of code, but not for the whole binary. | ||
|
|
||
| # There should be no space after -D CMAKE_BUILD_TYPE= but if we provide the environment variable inline, without | ||
| # the space it doesn't seem to get substituted... With invalid -D CMAKE_BUILD_TYPE argument cmake silently produces | ||
| # a DEBUG build. | ||
| if (($IsWindows) -or ((Test-Path Env:AGENT_OS) -and ($Env:AGENT_OS.StartsWith("Win")))) | ||
| { | ||
| Write-Host "On Windows build native simulator using the default C/C++ compiler (should be MSVC)" | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| $CMAKE_C_COMPILER = "-DCMAKE_C_COMPILER=clang.exe" | ||
| $CMAKE_CXX_COMPILER = "-DCMAKE_CXX_COMPILER=clang++.exe" | ||
|
|
||
| if ((!(Get-Command clang -ErrorAction SilentlyContinue) -and (choco find --idonly -l llvm) -contains "llvm") -or ` | ||
| (Test-Path Env:/AGENT_OS)) { | ||
| # LLVM was installed by Chocolatey, so add the install location to the path. | ||
| $env:PATH = "$($env:SystemDrive)\Program Files\LLVM\bin;$env:Path" | ||
| } | ||
|
|
||
| cmake -G Ninja -D BUILD_SHARED_LIBS:BOOL="1" $CMAKE_C_COMPILER $CMAKE_CXX_COMPILER ` | ||
| -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| # Without `-G Ninja` we fail to switch from MSVC to Clang. | ||
| # Sanitizers are not supported on Windows at the moment. Check again after migrating from Clang-11. | ||
| } | ||
| elseif (($IsLinux) -or ((Test-Path Env:AGENT_OS) -and ($Env:AGENT_OS.StartsWith("Lin")))) | ||
| { | ||
| Write-Host "On Linux build native simulator using gcc (needed for OpenMP)" | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_C_COMPILER=gcc -D CMAKE_CXX_COMPILER=g++ -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_C_COMPILER=clang-11 -D CMAKE_CXX_COMPILER=clang++-11 ` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like sanitizer flags are being added even for release builds, which we don't want. The sanitizers should only be enabled for debug builds or only in the sanitizer pipeline, we don't want to ship binaries on linux and mac that have sanitizers enabled.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not exactly, The sanitizer flags are only added for DEBUG, see below:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I misread, you are correct!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please uncheck the "Change requested" flag? |
||
| -D CMAKE_C_FLAGS_DEBUG="$SANITIZE_FLAGS" ` | ||
| -D CMAKE_CXX_FLAGS_DEBUG="$SANITIZE_FLAGS" ` | ||
| -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| } | ||
| elseif (($IsMacOS) -or ((Test-Path Env:AGENT_OS) -and ($Env:AGENT_OS.StartsWith("Darwin")))) | ||
| { | ||
| Write-Host "On MacOS build native simulator using gcc (needed for OpenMP)" | ||
| # `gcc`on Darwin seems to be a shim that redirects to AppleClang, to get real gcc, must point to the specific | ||
| # version of gcc we've installed. | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_C_COMPILER=gcc-7 -D CMAKE_CXX_COMPILER=g++-7 -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| Write-Host "On MacOS build using the default C/C++ compiler (should be AppleClang)" | ||
|
|
||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" ` | ||
| -D CMAKE_C_FLAGS_DEBUG="$SANITIZE_FLAGS" ` | ||
| -D CMAKE_CXX_FLAGS_DEBUG="$SANITIZE_FLAGS" ` | ||
| -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| } | ||
| else { | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuzminrobin Why did this path change? Seems like the CI should have had issues before or after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the native simulator was built with MSVC (on Win), the compiler was putting the binary to
Simulation/native/build/Release(whereas the GCC on other platforms was putting the binary toSimulation/native/build). Then the scripts were copying the binary toSimulation/native/build/drop(on all platforms), but the dependency in the tests was different for win and other platforms.After the native simulator has been migrated to Clang, on all the platforms the binary started being put
Simulation/native/buildand copied toSimulation/native/build/drop. This change has unified the dependency.