[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln added a comment. Note that all of this currently only matters when compiling with `-fsanitize=unreachable`. The following discussion is within the context of the current implementation: UBSan removes the `noreturn` so it can instrument `unreachable` without the added instrumentation being

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln added a comment. @lebedev.ri Thanks for the clarifications! I will split this up into multiple patches once we settled on a design. In D56624#1369635 , @vsk wrote: > What are the advantages of a generalized expect_noreturn attribute, vs. a >

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln abandoned this revision. yln added a comment. Created new revision for this change: https://reviews.llvm.org/D57278 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56624/new/ https://reviews.llvm.org/D56624 ___

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln added a comment. Seems as if we reached consensus! :) I will change the revision to use an intrinsic. Before I start doing that, just one more quick idea: Would it work if UBsan directly inserts calls to `__asan_handle_no_return` (of course only when ASan is requested). Similar to how it

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln added a comment. In D56624#1370579 , @eugenis wrote: > Maybe the frontend should insert __asan_handle_noreturn whenever ASan is > enabled, and then ASan would not care about the attribute? I'd like to avoid > having this logic in two places. +1

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln added a comment. In D57278#1374852 , @vsk wrote: > Is it necessary to remove visitation of 'noreturn' call sites from ASan? I'd > expect that to break non-C frontends which emit noreturn calls (rust/swift?). Good point! I didn't think about other

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln updated this revision to Diff 184168. yln added a comment. Directly insert calls to `__asan_handle_no_return` Clang-CodeGen now directly insert calls to `__asan_handle_no_return` when a call to a noreturn function is encountered and both UBsan unreachable and ASan are enabled. This allows

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln added a comment. @eugenis @delcypher Looks good to you? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57278/new/ https://reviews.llvm.org/D57278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D57489: [ASan] Do not instrument other runtime functions with `__asan_handle_no_return`

This revision was automatically updated to reflect the committed changes. Closed by commit rC352948: [ASan] Do not instrument other runtime functions with `__asan_handle_no_return` (authored by yln, committed by ). Herald added a subscriber: cfe-commits. Changed prior to commit:

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

This revision was automatically updated to reflect the committed changes. Closed by commit rC352690: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of… (authored by yln, committed by ). Changed prior to commit: https://reviews.llvm.org/D57278?vs=184168=184393#toc

[PATCH] D57711: [Sanitizers] UBSan unreachable incompatible with Kernel ASan

This revision was automatically updated to reflect the committed changes. Closed by commit rC353120: [Sanitizers] UBSan unreachable incompatible with Kernel ASan (authored by yln, committed by ). Changed prior to commit: https://reviews.llvm.org/D57711?vs=185153=185178#toc Repository: rC

[PATCH] D57722: [Clang][NCF] Sanitizer options: helpers to check if {Kernel, Hardware}ASan is enabled

yln created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Kernel ASan is basically ASan with a few specific settings, but has a seperate front-end flag. Provide a helper that checks for both, so new comitters like myself are less likely to forget the Kernel

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

yln reopened this revision. yln added a comment. In D56624#1368966 , @lebedev.ri wrote: > Please revert this. > First, this wasn't reviewed. > Second, the lists weren't subscribed. I apologize for this. It was not my intention to land the revision

[PATCH] D57711: [Sanitizers] UBSan unreachable incompatible with Kernel ASan

yln created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is a follow up for https://reviews.llvm.org/D57278. The previous revision should have also included Kernel ASan. rdar://problem/40723397 Repository: rG LLVM Github Monorepo

[PATCH] D86980: [Docs] Remove outdated OS limitation

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG478eb98cd25c: [Docs] Remove outdated OS limitation (authored by yln). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D86980: [Docs] Remove outdated OS limitation

yln created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. yln requested review of this revision. Thread Sanitizer is supported on macOS. rdar://68159753 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D86980 Files:

[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

yln added inline comments. Comment at: llvm/docs/CommandGuide/lit.rst:102 + + Alias for ``-v``/``--verbose`` (for backwards compatibility). .. option:: -a, --show-all blank line Comment at: llvm/utils/lit/lit/OutputSettings.py:34

[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

yln added a comment. The changes here are for text printed for failing tests, right? And `showAllOutput` is orthogonal and just shows everything even for non-failing tests? Comment at: llvm/utils/lit/lit/OutputSettings.py:34 +ONLY_FAILING_COMMAND =

[PATCH] D77708: [lit] Improve naming of test result categories

This revision was automatically updated to reflect the committed changes. Closed by commit rG99d6e05e7144: [lit] Improve naming of test result categories (authored by yln). Herald added subscribers: cfe-commits, msifontes, jurahul, Kayjukh, frgossen, grosul1, Joonsoo, stephenneuendorffer,

[PATCH] D84082: [tsan] Allow TSan in the Clang driver for Apple Silicon Macs

yln added a comment. Simulators on Apple Silicon are not yet supported? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84082/new/ https://reviews.llvm.org/D84082 ___ cfe-commits mailing list

[PATCH] D84082: [tsan] Allow TSan in the Clang driver for Apple Silicon Macs

yln added inline comments. Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2734 - if (isTargetMacOS()) { -if (IsX86_64) - Res |= SanitizerKind::Thread; + if ((IsX86_64 || IsAArch64) && isTargetMacOS()) { +Res |= SanitizerKind::Thread; This

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

yln added a comment. My apologies for misrepresenting the meaning of the docs and stirring up this confusion. I did not read the docs carefully enough. Replying on the mailing list ... Comment at: clang/CMakeLists.txt:1 cmake_minimum_required(VERSION 3.13.4)

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

yln added a comment. In D95635#2528851 , @JDevlieghere wrote: >> However, the project claims to require 3.6 or greater, and 3.6 features are >> being used. > > Can you link to where it says that? I'm not opposed to making 3.6 the > minimally required

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

yln added a comment. LGTM, thanks! (I got bitten by this.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95635/new/ https://reviews.llvm.org/D95635 ___ cfe-commits mailing list

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

yln accepted this revision. yln added a comment. This revision is now accepted and ready to land. LGTM, cleanup only, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97327/new/ https://reviews.llvm.org/D97327

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

yln added inline comments. Comment at: clang/lib/CodeGen/CGAtomic.cpp:444-447 +// Prior to c++17, "the failure argument shall be no stronger than the +// success argument". This condition has been lifted and the only +// precondition is 31.7.2.18. Effectively treat

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

yln added inline comments. Comment at: clang/lib/CodeGen/CGAtomic.cpp:444-447 +// Prior to c++17, "the failure argument shall be no stronger than the +// success argument". This condition has been lifted and the only +// precondition is 31.7.2.18. Effectively treat

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. I've added 2 additional calls to `initializeLowerGlobalDtorsLegacyPassPass()`. The one in the constructor of the legacy pass should ensure that the pass is always registered. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. Plan to land tomorrow morning. Test passes on the Debian bot : PASS: LLVM :: Transforms/LowerGlobalDestructors/lower-global-dtors.ll (72299 of 93291) Repository: rG

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln updated this revision to Diff 415273. yln added a comment. Herald added subscribers: cfe-commits, ormris. Herald added a project: clang. Add support for an escape hatch to fallback to the old behavior: via Clang driver flag (`-fregister-global-dtors-with-atexit`) or llc / code generation

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln marked 2 inline comments as done. yln added inline comments. Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:900 + // __cxa_atexit calls to avoid emitting the deprecated __mod_term_func. + if (TM->getTargetTriple().isOSBinFormatMachO()) +

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. @sunfish Hi Dan, I hope you are still happy with this change. I didn't change any WebAssembly tests, but rather added a new IR-level test, so all existing WebAssembly behavior should stay the same. Let me know if you have any concerns. Repository: rG LLVM Github

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. yln marked an inline comment as done. Closed by commit rG9c542a5a4e1b: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO (authored by yln). Repository: rG

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln marked an inline comment as done. yln added inline comments. Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:900 + // __cxa_atexit calls to avoid emitting the deprecated __mod_term_func. + if (TM->getTargetTriple().isOSBinFormatMachO()) +

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln updated this revision to Diff 415282. yln added a comment. Report fatal error when we can, instead of never. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121327/new/ https://reviews.llvm.org/D121327 Files: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln created this revision. yln added reviewers: Orlando, RKSimon. Herald added subscribers: asb, ormris, pengfei, sunfish, hiraditya, jgravelle-google, sbc100, mgorny, dschuff. Herald added a project: All. yln requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits,

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. New revision here: D121736 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121327/new/ https://reviews.llvm.org/D121327 ___ cfe-commits mailing list

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. Hi @Orlando and @RKSimon, would you be able to confirm that this version avoids the previous `opt: Unknown command line argument '-lower-global-dtors'` error? Lit invocation: `env LIT_FILTER='dtor' ninja check-llvm` Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. Hi @Orlando and @RKSimon! Thanks for pointing out the test failure and reverting the change. I see this failure: : 'RUN: at line 1'; /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/opt -lower-global-dtors -S <

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. It looks like all tests succeed for `x64 debian`: https://buildkite.com/llvm-project/premerge-checks/builds/83802#c5002be5-e01c-46fc-a767-50fe6f854465 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121736/new/

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. In D121736#3389251 , @RKSimon wrote: > LGTM - but please hang around after you commit Thanks! Looks like this it worked out. Let me know if there is any other problems. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

yln added a comment. Thank you @zequanwu, for providing the minimized reproducer! It really made fixing this much easier! :) I've re-landed the change and added a test specifically for this issue: `lower-global-dtors-existing-dos_handle.ll` Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG22570bac6943: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO (authored by yln). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

yln added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2349 + if (!cookieOffset.isZero()) +cookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, cookieOffset); Variable names should start with uppercase:

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

yln added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2453 +CGF.Builder.CreateCall(F, cookie.getRawPointer(CGF)); + } This code is very similar to what we have in `ItaniumCXXABI::InitializeArrayCookie()`. Can we try to extract a

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

yln added inline comments. Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262 *reinterpret_cast(s) = kAsanArrayCookieMagic; + // The ARM64 cookie has a second "elementSize" entry so poison it as well + #if SANITIZER_ARM64 yln wrote: > Nitpicking

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

yln added inline comments. Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262 *reinterpret_cast(s) = kAsanArrayCookieMagic; + // The ARM64 cookie has a second "elementSize" entry so poison it as well + #if SANITIZER_ARM64 yln wrote: > yln wrote: > >

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

yln added inline comments. Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:263 +#if SANITIZER_ARM64 + // The ARM64 cookie has a second "size_t" entry so poison it as well + *(reinterpret_cast(s)-1) = kAsanArrayCookieMagic; Comment at:

[PATCH] D139652: Add the thread sanitizer support for X86_64 WatchOS simulators

yln accepted this revision. yln added a subscriber: rsundahl. yln added a comment. Thank you! LGTM, with one nit. @rsundahl: can you keep an eye on this and resolve potential conflicts? Thanks! Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:3280-3286 if ((IsX86_64 ||

[PATCH] D142421: [Sanitizers] fix -fno-sanitize-link-runtime for darwin

yln added a comment. Looks harmless enough, even though I am not sure why we have a completely separate code path on Darwin. The other toolchain driver seem to have shared code for this in `clang/lib/Driver/ToolChains/CommonArgs.cpp`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D137484: [Test] Fix driverkit-path.c with lib64 dir

yln accepted this revision. yln added a comment. This revision is now accepted and ready to land. Looks harmless enough.. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137484/new/ https://reviews.llvm.org/D137484

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit

yln created this revision. Herald added subscribers: ormris, hiraditya. Herald added a project: All. yln requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Remove the `-lower-global-dtors-via-cxa-atexit` escape hatch

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

yln added inline comments. Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1205 unsigned Priority, const MCSymbol *KeySym) const { - // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag - // (LowerGlobalDtorsViaCxaAtExit) and always issue a

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

yln added a comment. Mentioned removal of flag in release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145715/new/ https://reviews.llvm.org/D145715 ___ cfe-commits mailing list

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

yln updated this revision to Diff 504890. yln marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145715/new/ https://reviews.llvm.org/D145715 Files: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

yln added a comment. I will land this later today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145715/new/ https://reviews.llvm.org/D145715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe6a789ef9bb2: Remove -lower-global-dtors-via-cxa-atexit flag (authored by yln). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

yln added inline comments. Comment at: compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11 + +// darwin only supports shared-libsan, so this should fail. +// XFAIL: darwin + This should work, right? Repository: rG LLVM Github Monorepo

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

yln added inline comments. Comment at: compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11 + +// darwin only supports shared-libsan, so this should fail. +// XFAIL: darwin + zixuw wrote: > dmaclach wrote: > > yln wrote: > > > This should

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

yln added inline comments. Comment at: compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11 + +// darwin only supports shared-libsan, so this should fail. +// XFAIL: darwin + usama54321 wrote: > yln wrote: > > zixuw wrote: > > > dmaclach

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

yln accepted this revision. yln added inline comments. Comment at: compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11 + +// darwin only supports shared-libsan, so this should fail. +// XFAIL: darwin + usama54321 wrote: > yln wrote: > >

[PATCH] D148967: WIP: Disable atexit()-based lowering when LTO'ing kernel/kext code

yln created this revision. Herald added subscribers: llvm-commits, cfe-commits, ormris, steven_wu, hiraditya, inglorion. Herald added projects: clang, LLVM, All. yln requested review of this revision. Herald added a subscriber: MaskRay. rdar://93536111 Repository: rG LLVM Github Monorepo

[PATCH] D148967: Disable atexit()-based lowering when LTO'ing kernel/kext code

This revision was automatically updated to reflect the committed changes. Closed by commit rGc3f0153ec27a: [MachO] Disable atexit()-based lowering when LTOing kernel/kext code (authored by yln). Changed prior to commit: https://reviews.llvm.org/D148967?vs=515929=516862#toc Repository: rG

[PATCH] D157445: [CodeGen][UBSan] Add support for handling attributed functions in getUBSanFunctionTypeHash.

yln added inline comments. Comment at: clang/test/CodeGen/ubsan-function-attributed.c:4 +//Make sure compiler does not crash inside getUBSanFunctionTypeHash while compiling this +long __attribute__((ms_abi)) f() {} MaskRay wrote: >