[Asan] Skip pre-split coroutine and noop coroutine frame

[apolloww]

CoroSplit expects the second parameter of llvm.coro.id to be the promise alloca. Applying Asan on a pre-split coroutine breaks this assumption and causes split to fail. This should be NFC because asan pass happens late in the pipeline where all coroutines are split. This is to prevent crash in case the order of passes are switched.

Also NoopCoro.Frame.Const is a special coroutine frame that does nothing when resumed or destroyed. There is no point to do instrumentation on it.

[llvmbot]

@llvm/pr-subscribers-coroutines
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Wei Wang (apolloww)

Changes

CoroSplit expects the second parameter of llvm.coro.id to be the promise alloca. Applying Asan on a pre-split coroutine breaks this assumption and causes split to fail.

Also NoopCoro.Frame.Const is a special coroutine frame that does nothing when resumed or destroyed. There is no point to do instrumentation on it.

---

Full diff: https://github.com/llvm/llvm-project/pull/99415.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+2)
• (added) llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll (+36)

diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index adf77f20cb1c7..89cba2b219c25 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -2032,6 +2032,7 @@ bool ModuleAddressSanitizer::shouldInstrumentGlobal(GlobalVariable *G) const {
   if (G->isThreadLocal()) return false;
   // For now, just ignore this Global if the alignment is large.
   if (G->getAlign() && *G->getAlign() > getMinRedzoneSizeForGlobal()) return false;
+  if (G->getName() == "NoopCoro.Frame.Const") return false;
 
   // For non-COFF targets, only instrument globals known to be defined by this
   // TU.
@@ -2947,6 +2948,7 @@ bool AddressSanitizer::instrumentFunction(Function &F,
   if (F.getLinkage() == GlobalValue::AvailableExternallyLinkage) return false;
   if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) return false;
   if (F.getName().starts_with("__asan_")) return false;
+  if (F.isPresplitCoroutine()) return false;
 
   bool FunctionModified = false;
 
diff --git a/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll b/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll
new file mode 100644
index 0000000000000..7aaf6ee8c471b
--- /dev/null
+++ b/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll
@@ -0,0 +1,36 @@
+; Tests that asan skips pre-split coroutine and NoopCoro.Frame
+; RUN: opt < %s -S -passes=coro-early,asan | FileCheck %s
+
+; CHECK: %NoopCoro.Frame = type { ptr, ptr }
+; CHECK: @NoopCoro.Frame.Const = private constant %NoopCoro.Frame { ptr @__NoopCoro_ResumeDestroy, ptr @__NoopCoro_ResumeDestroy }
+; CHECK-NOT: @0 = private alias { %NoopCoro.Frame,
+
+%struct.Promise = type { %"struct.std::__n4861::coroutine_handle" }
+%"struct.std::__n4861::coroutine_handle" = type { ptr }
+
+; CHECK-LABEL: @foo(
+define ptr @foo() #0 {
+; CHECK-NEXT: entry
+; CHECK-NOT: %asan_local_stack_base
+entry:
+  %__promise = alloca %struct.Promise, align 8
+  %0 = call token @llvm.coro.id(i32 16, ptr nonnull %__promise, ptr nonnull @foo, ptr null)
+  %1 = call ptr @llvm.coro.noop()
+  ret ptr %1
+}
+
+declare token @llvm.coro.id(i32, ptr readnone, ptr nocapture readonly, ptr)
+declare ptr @llvm.coro.noop()
+
+attributes #0 = { sanitize_address presplitcoroutine }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "hand-written", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "<stdin>", directory: "")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+
+

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vitalybuka]

Seems attaching attach metadata on the global is nicer solution?
See line 2020

[vitalybuka]

Also there is

if (F.isPresplitCoroutine())
    return false;

[apolloww]

Thanks for the suggestion. Updated the GV metadata. I'll leave the function metadata unchanged because we still want to instrument post-split coroutine functions, and isPresplitCoroutine() can reflect the state.

[vogelsgesang]

nit: for consistency with preceding lines:

Suggested change

	if (F.isPresplitCoroutine())
	return false;
	if (F.isPresplitCoroutine()) return false;

[apolloww]

They were on the same line, and I don't see a problem with that (see the first commit), but clang-format complained.