Enable sanitizers for macOS, Linux, Windows #1559
Conversation
|
something I ran into 5 years ago that's still a big challenge: getting ASan to run on Android simulator/device is very very finicky. If you manage to figure it out, it would be a huge help! :D if that's what you get stuck on, then I'd say that getting v8 integration coverage on a non-Android platform could be a good fallback that would likely give the effectively the same feedback loop. |
|
@matthargett I think you are referring to ASAN images not available and missing .so :) |
…nto ASAN # Conflicts: # Polyfills/Canvas/Source/Gradient.cpp
CedricGuillemet
changed the title
…nto ASAN # Conflicts: # Apps/UnitTests/JavaScript/webpack.config.js # Apps/package-lock.json
CedricGuillemet
changed the title
…nto ASAN # Conflicts: # Apps/package-lock.json
| - script: | | ||
| cd build/Apps/UnitTests | ||
| xvfb-run ./UnitTests | ||
| displayName: 'Unit Tests' | ||
There was a problem hiding this comment.
Did you intend to move this?
| - powershell: | | ||
| $vs = vswhere -latest -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath | ||
| $msvc = Get-ChildItem "$vs\VC\Tools\MSVC" | Sort-Object Name -Descending | Select-Object -First 1 | ||
| $asan = "$($msvc.FullName)\bin\Hostx64\x64" | ||
| $destinations = @( | ||
| "build${{ variables.solutionName }}\Apps\Playground\RelWithDebInfo", | ||
| "build${{ variables.solutionName }}\Apps\UnitTests\RelWithDebInfo" | ||
| ) | ||
| foreach ($dest in $destinations) { | ||
| Get-ChildItem "$asan\clang_rt.asan_*.dll" | | ||
| Copy-Item -Destination $dest | ||
| } | ||
| displayName: "Copy ASAN runtime DLLs" | ||
| condition: ${{ parameters.enableSanitizers }} |
There was a problem hiding this comment.
This is fine for now. Consider doing this inside CMake since it seems like it's part of the build.
There was a problem hiding this comment.
Not sure. These dll are in PATH when debugging/testing with Visual Studio.
There was a problem hiding this comment.
Hmm, maybe we should do the same and add them to the path? Do you know what adds them to the path?
There was a problem hiding this comment.
nit: Maybe this file should go in the source and copied with the app so that you don't have to do the ../../.. thing.
| // context is a monitoredResource managed by the GC while Canvas::Impl is managed by the app. Make sure the canvas impl is destroyed after all monitoredResources are. | ||
| std::shared_ptr<Canvas::Impl> m_impl; |
There was a problem hiding this comment.
I don't know this code well. Is it okay to have different lifetime management for this?
| # MSVC only supports AddressSanitizer | ||
| add_compile_options(/fsanitize=address /Zi /Od) | ||
| else() | ||
| message(WARNING "Sanitizers not supported on this compiler.") |
There was a problem hiding this comment.
Maybe this should be an error instead of warning? I would think this is a mistake if someone tried to enable sanitizers on a platform that doesn't allow it.
| - template: .github/jobs/win32.yml | ||
| parameters: | ||
| name: Win32_x64_D3D12_Sanitizers | ||
| vmImage: 'windows-latest' | ||
| platform: x64 | ||
| graphics_api: D3D12 | ||
| enableSanitizers: true | ||
|
|
||
| - template: .github/jobs/win32.yml | ||
| parameters: | ||
| name: Win32_x64_V8_D3D11_Sanitizers | ||
| vmImage: 'windows-latest' | ||
| platform: x64 | ||
| napiType: V8 | ||
| enableSanitizers: true |
There was a problem hiding this comment.
How much time does this add to the CI if any? Do we need to run both with sanitizers and without?
There was a problem hiding this comment.
1 should be enough, indeed.
3 participants
Enable Address sanitizer as pointed by #1557
Memleaks + CI issue : #1575