[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
https://github.com/barcharcraz edited https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: @rnk @vitalybuka This should be ready for a more in-depth review (I'll stop force-pushing) The remaining commits here all depend on each-other in some way. Splitting off any more commits would mean introducing bugs into main until other parts of this are merged. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: @vitalybuka Here's a more detailed explination on the motivations behind this change from @amyw-msft, who is the original author of these changes on our side. (And who I'll add as a co-author using fixup commits) https://devblogs.microsoft.com/cppblog/msvc-address-sanitizer-one-dll-for-all-runtime-configurations/ https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote:
> > @vitalybuka Here's a more detailed explination on the motivations behind
> > this change from @amyw-msft, who is the original author of these changes on
> > our side. (And who I'll add as a co-author using fixup commits)
> > https://devblogs.microsoft.com/cppblog/msvc-address-sanitizer-one-dll-for-all-runtime-configurations/
>
> Thanks. Would be possible to extract summary paragraph and put on top of the
> main patch, with this URL.
>
> Also, to submit, it would be nice to do with smaller patches with some time
> between them to let bots test them separately. Or it's going to be annoying
> to revert/reland such large patch.
>
> Almost every item from description looks like a candidate for a patch.
>
> Relaxing `// CHECK: #{{[1-3]}}` could be done in a separate patch.
OK I'll extract at least the first few commits into their own PR, along with
the test relaxations. The items in the description are harder to extract
without breaking all the tests.
https://github.com/llvm/llvm-project/pull/81677
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: > Is it feasible to create a migration path from the static runtime to the > dynamic runtime? As written, this requires users to update their ASan builds > at the same time that they take this update. Is it possible to leave the > static runtime behind, create a migration path to the dynamic runtime (add > all the new weak symbol registration functionality), push that, and then > follow up by removing the static runtime 2-4 weeks later? The new symbol registration mechanism depends on how the loader imports multiple DLLexports of the same symbol, maybe it could be adapted to work with the static runtime, but it's a bunch of work for something that will be immediately removed. We could also just ship a copy of the dynamic runtime import library under the same name as the old static runtime, perhaps emitting a warning someplace to let people know what's going on. So, for context: MSVC's link.exe uses a special /INFERASANLIBS option that picks the correct asan library automatically, and this PR should (hopefully, I haven't tested it yet) make that flag work with clang's version of asan as well, at least for release builds (MSVC ships with debug builds of asan as well, they're still linked to the release-dll variant of the CRT, they just have optimization turned off). When you pass /fsanitize=address to cl.exe it adds the equivalent of `#pragma comment(linker, "/INFERASANLIBS")` to the object files. Link.exe sees this and picks the right libraries by seeing which crt it was going to link and inserting the asan libraries before the crt in the link order. I'm not quite sure I understand the problem scenario. Is the trouble that if you're building with mingw's gcc then you need to invoke the linker manually because gcc doesn't know how to deal with link.exe (and you want to use link.exe over ld.exe because ld is slow and presumably not that well maintained on windows?). I think clang can invoke link.exe for you, just like any other linker, or is that only lld-link? cl can also invoke link for you, although I understand going through the compiler like that is less common on windows that on other platforms. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: > This is long description, but it does not explain WHY. Could you please add > some explanation there? Sure, the reason is to greatly simplify how asan works on windows, and to fix a bunch of bugs with asan+static CRT. Before this change there was a bunch of code that would redirect calls from various dlls into the executable if the asan runtime was statically linked, this code has never worked all that well, and it was a nightmare to fix. Additionally, statically linked DLLs didn't work with the previous scheme and weak symbols didn't work with /MD. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: > I think @rnk has the deepest knowledge here. > > Just to double check, when you say "Eliminate the static asan runtime", you > mean the use case it was supporting will still keep working, but in a > different way right? Yes. The static asan runtime didn't work very well in the first place, so this actually improves support for those use-cases. The static runtime is still supported on non-windows systems, however. The whole static linked asan runtime library is gone on windows, but you can still use the dynamic asan runtime with a statically linked CRT. The core reasoning is that asan is a "only one allowed per process" type thing (you can't have one copy of the asan runtime handling a malloc while a different one handles the corresponding free). To get this on windows you really need to use a DLL or some really, really horrible hacks that essentially amounted to doing yourself what the loader already does for DLLs. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: > > The core reasoning is that asan is a "only one allowed per process" type > > thing (you can't have one copy of the asan runtime handling a malloc while > > a different one handles the corresponding free). > > Not to distract from the direction taken here (which I do agree seems > reasonable, even if I haven't had time to look closer at the PR yet) - but, > when using the static CRT in general, doesn't the same concern apply there as > well? I.e. when using the static CRT, care must be taken that the same CRT > instances does frees as did the allocation. But I guess there are other > asan-related aspects that makes it even more of a mess. Yes, it does, but my understanding is that this requirement is slightly weaker than what asan needs. As long as you _do_ match mallocs and frees things work fine, because the data that's shared is shared through kernel32.dll, ntdll.dll, or the kernel and page table. If asan allowed this it would need some way of mapping memory to the correct global structures, and some way of figuring out who gets to handle the shadow memory area and where to get the stack information for a given allocation upon an invalid access. We can't rely on "matching" allocations with invalid accesses because it's far from uncommon for a memory error to relate to memory allocated in another DLL. Maybe the static-linked asans could have some voting mechanism to settle on a "lead" copy of asan or something. It's not impossible but making things work well is a ton of work for something the loader can do anyway. Before this change we still relied on only one copy of asan being around, but the functions would be exported from the main exe with the "dll thunk" using GetProcAddress to call them. > Also secondly, when discussing these details - how the asan runtime is built > in one, specific way, while it is used for applications that can use a > different CRT configuration It might be possible to support copies of the asan runtime built with the static CRT, it's just not that useful and we'd rather keep one configuration and spend time making it work perfectly for all instrumented programs. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: It occurs to me that this probably requires changes to the gyp build files. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
https://github.com/barcharcraz ready_for_review https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
barcharcraz wrote: The buildkite failure is just windows defender freaking out about one of cmake's test exes. Presumably it'll get sorted out next time it runs. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Reland [asan][windows] Eliminate the static asan runtime on windows (PR #107899)
https://github.com/barcharcraz closed https://github.com/llvm/llvm-project/pull/107899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
https://github.com/barcharcraz edited https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
https://github.com/barcharcraz closed https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #93770)
https://github.com/barcharcraz edited https://github.com/llvm/llvm-project/pull/93770 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #93770)
https://github.com/barcharcraz closed https://github.com/llvm/llvm-project/pull/93770 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [asan][windows] Eliminate the static asan runtime on windows (PR #81677)
@@ -35,6 +35,9 @@ // RUN: %p/../../../../lib/sanitizer_common/sanitizer_coverage_interface.inc \ // RUN: | grep -e "INTERFACE_\(WEAK_\)\?FUNCTION" \ // RUN: | grep -v "__sanitizer_weak_hook" \ +// RUN: | grep -v "__sanitizer_override_function" \ barcharcraz wrote: It would cause test failures on linux/darwin. These tests grep the interface files for "expected" imports and do not run the preprocessor on them, so the new interface symbols on windows need to be excluded. https://github.com/llvm/llvm-project/pull/81677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Reland [asan][windows] Eliminate the static asan runtime on windows (PR #107899)
barcharcraz wrote: > This PR seems to have broken ASAN in mingw configurations. The symptoms seem > to be that the ASAN DLL just locks up, hard, when the process is loaded. (Or > more precisely, has entered some infinite loop.) > > I tried attaching to such a hung process with windbg, and it's showing this: > > ``` > Break-in sent, waiting 30 seconds... > WARNING: Break-in timed out, suspending. > This is usually caused by another thread holding the loader lock. > ``` > > And a backtrace that looks like this: > > ``` > libclang_rt_asan_dynamic_x86_64!_asan_default_options__dll > libclang_rt_asan_dynamic_x86_64!_asan_default_options__dll+0xa17 > libclang_rt_asan_dynamic_x86_64!_sanitizer_register_weak_function+0xd2 > stacksmash_asan+0x1dad > stacksmash_asan+0x16b7 > stacksmash_asan+0x20e3 > ntdll!LdrpCallInitRoutine+0x6f > ntdll!... > ``` > > (Side note; the PR seems to have added a bunch of new files with CRLF > newlines - it'd be nice to clean this up.) > > To repro the issue for yourself, you can do the following: > > 1. Download and unzip > https://github.com/mstorsjo/llvm-mingw/releases/download/20240903/llvm-mingw-20240903-ucrt-x86_64.zip, > and add the `llvm-mingw--ucrt-x86_64\bin` directory to your > `%PATH%` within a terminal > 2. Configure a build of compiler-rt with the following parameters: `cmake > ..\compiler-rt -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=clang > -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER_TARGET=x86_64-w64-windows-gnu > -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE > -DCOMPILER_RT_USE_BUILTINS_LIBRARY=TRUE -DSANITIZER_CXX_ABI=libc++ > -DCMAKE_INSTALL_PREFIX=c:\code\llvm-mingw-20240903-ucrt-x86_64\lib\clang\19` > 3. Build and install (on top of the newly unpacked toolchain), `ninja install` > 4. Copy the newly built asan DLL into the current directory, `copy /y > lib\windows\libclang_rt.asan_dynamic-x86_64.dll .` > 5. Compile a trivial hello world, like > https://github.com/mstorsjo/llvm-mingw/blob/master/test/hello.c (really any > snippet will do), with asan `clang hello.c -o hello.exe -fsanitize=address` > 6. Try to run `hello.exe`, which hangs. > > It's possible to reproduce the same by building and running the whole > compiler-rt testsuite as well, but that's a bit trickier to set up for this > configuration. > > Surprising side note; when I tried the repro procedure above, building > compiler-rt with a slightly older Clang release, and 18.x version, the built > ASAN seems to not hang. I'm not sure if this is a functional difference, or > if it just so happens to link things slightly differently so whatever issue > there is seems to just not happen. > > This commit is kinda complex, as it not only removes the static asan > configuration (which never was involved in mingw use cases before), but I > guess also sets things up so the dynamic asan can be used even when linking > the CRT statically? > > This issue has caused my nightly builds to start failing: > https://github.com/mstorsjo/llvm-mingw/actions/runs/10803005560 After investigating it is indeed mangled code due to the UB in register_weak_. I had to step through from the very beginning of process init, as it doesn't seem like the debuginfo generated by `-gcodeview` points to the right place, but anyway for `__asan_default_options_dll` the codegen for the weak registration function looks as follows ```asm 7ff6`90501440 4883ec28 sub rsp, 28h 7ff6`90501444 488d0d752c lea rcx, [hello!.refptr._newmode+0x38 (7ff6905040c0)] 7ff6`9050144b 488d159e1f lea rdx, [hello!__asan_default_options__dll (7ff6905033f0)] 7ff6`90501452 e8e906 call hello!_ZN11__sanitizer13register_weakEPKcy (7ff690501b40) 7ff6`90501457 89442424 mov dword ptr [rsp+24h], eax 7ff6`9050145b 8b442424 mov eax, dword ptr [rsp+24h] 7ff6`9050145f 4883c428 add rsp, 28h 7ff6`90501463 c3 ret ``` Note that there is no comparison, the call to __sanitizer::register_weak is made unconditionally, even though in this case the local function should be the same as the default implementation from the dll. Worse, it seems like it's not even loading the correct pointer for the local function, so it ends up "intercepting" some random memory! I've fixed this in https://github.com/llvm/llvm-project/pull/108327 https://github.com/llvm/llvm-project/pull/107899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Reland [asan][windows] Eliminate the static asan runtime on windows (PR #107899)
@@ -136,7 +160,7 @@ append_list_if(MINGW "${MINGW_LIBRARIES}" ASAN_DYNAMIC_LIBS)
add_compiler_rt_object_libraries(RTAsan_dynamic
OS ${SANITIZER_COMMON_SUPPORTED_OS}
ARCHS ${ASAN_SUPPORTED_ARCH}
- SOURCES ${ASAN_SOURCES} ${ASAN_CXX_SOURCES}
+ SOURCES ${ASAN_SOURCES}
barcharcraz wrote:
I've opened https://github.com/llvm/llvm-project/pull/108329 which should
revert this particular edit (which was arguably a driveby change anyway)
https://github.com/llvm/llvm-project/pull/107899
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
