Skip to content

Conversation

cjappl
Copy link
Contributor

@cjappl cjappl commented Aug 20, 2024

This new attribute should be incompatible with the sanitize_realtime attribute.

@cjappl cjappl requested review from nikic and vitalybuka August 20, 2024 23:09
@cjappl
Copy link
Contributor Author

cjappl commented Aug 20, 2024

CC Co-author @davidtrevelyan

@llvmbot
Copy link
Member

llvmbot commented Aug 20, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Chris Apple (cjappl)

Changes

This new attribute should be incompatible with the sanitize_realtime attribute.


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

10 Files Affected:

  • (modified) llvm/docs/LangRef.rst (+5)
  • (modified) llvm/include/llvm/Bitcode/LLVMBitCodes.h (+1)
  • (modified) llvm/include/llvm/IR/Attributes.td (+3)
  • (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+2)
  • (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+2)
  • (modified) llvm/lib/IR/Verifier.cpp (+6)
  • (modified) llvm/lib/Transforms/Utils/CodeExtractor.cpp (+1)
  • (modified) llvm/test/Bitcode/attributes.ll (+7)
  • (modified) llvm/test/Bitcode/compatibility.ll (+6-2)
  • (added) llvm/test/Verifier/rtsan-attrs.ll (+9)
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 445980a18e7e93..4887ae6ee99668 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -2189,6 +2189,10 @@ example:
 ``nosanitize_coverage``
     This attribute indicates that SanitizerCoverage instrumentation is disabled
     for this function.
+``nosanitize_realtime``
+    This attribute indicates that the Realtime Sanitizer instrumentation is
+    disabled for this function.
+    This attribute is incompatible with the ``sanitize_realtime`` attribute.
 ``null_pointer_is_valid``
    If ``null_pointer_is_valid`` is set, then the ``null`` address
    in address-space 0 is considered to be a valid address for memory loads and
@@ -2315,6 +2319,7 @@ example:
     This attribute indicates that RealtimeSanitizer checks
     (realtime safety analysis - no allocations, syscalls or exceptions) are enabled
     for this function.
+    This attribute is incompatible with the ``nosanitize_realtime`` attribute.
 ``speculative_load_hardening``
     This attribute indicates that
     `Speculative Load Hardening <https://llvm.org/docs/SpeculativeLoadHardening.html>`_
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index 4beac37a583445..8a2e6583af87c5 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -759,6 +759,7 @@ enum AttributeKindCodes {
   ATTR_KIND_INITIALIZES = 94,
   ATTR_KIND_HYBRID_PATCHABLE = 95,
   ATTR_KIND_SANITIZE_REALTIME = 96,
+  ATTR_KIND_NO_SANITIZE_REALTIME = 97,
 };
 
 enum ComdatSelectionKindCodes {
diff --git a/llvm/include/llvm/IR/Attributes.td b/llvm/include/llvm/IR/Attributes.td
index 891e34fec0c798..80936c0ee83355 100644
--- a/llvm/include/llvm/IR/Attributes.td
+++ b/llvm/include/llvm/IR/Attributes.td
@@ -212,6 +212,9 @@ def NoSanitizeBounds : EnumAttr<"nosanitize_bounds", [FnAttr]>;
 /// No SanitizeCoverage instrumentation.
 def NoSanitizeCoverage : EnumAttr<"nosanitize_coverage", [FnAttr]>;
 
+/// No SanitizeRealtime instrumentation.
+def NoSanitizeRealtime : EnumAttr<"nosanitize_realtime", [FnAttr]>;
+
 /// Null pointer in address space zero is valid.
 def NullPointerIsValid : EnumAttr<"null_pointer_is_valid", [FnAttr]>;
 
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index d4dbab04e8ecdb..6767c7ca288bc4 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -2093,6 +2093,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
     return Attribute::NoSanitizeBounds;
   case bitc::ATTR_KIND_NO_SANITIZE_COVERAGE:
     return Attribute::NoSanitizeCoverage;
+  case bitc::ATTR_KIND_NO_SANITIZE_REALTIME:
+    return Attribute::NoSanitizeRealtime;
   case bitc::ATTR_KIND_NULL_POINTER_IS_VALID:
     return Attribute::NullPointerIsValid;
   case bitc::ATTR_KIND_OPTIMIZE_FOR_DEBUGGING:
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 03d0537291dada..de24dcdc27e266 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -795,6 +795,8 @@ static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
     return bitc::ATTR_KIND_NO_SANITIZE_BOUNDS;
   case Attribute::NoSanitizeCoverage:
     return bitc::ATTR_KIND_NO_SANITIZE_COVERAGE;
+  case llvm::Attribute::NoSanitizeRealtime:
+    return bitc::ATTR_KIND_NO_SANITIZE_REALTIME;
   case Attribute::NullPointerIsValid:
     return bitc::ATTR_KIND_NULL_POINTER_IS_VALID;
   case Attribute::OptimizeForDebugging:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index c095e47996ba17..e65ed7ee8b3f10 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2223,6 +2223,12 @@ void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
           "Attributes 'optdebug and optnone' are incompatible!", V);
   }
 
+  Check(!(Attrs.hasFnAttr(Attribute::SanitizeRealtime) &&
+          Attrs.hasFnAttr(Attribute::NoSanitizeRealtime)),
+        "Attributes "
+        "'sanitize_realtime and nosanitize_realtime' are incompatible!",
+        V);
+
   if (Attrs.hasFnAttr(Attribute::OptimizeForDebugging)) {
     Check(!Attrs.hasFnAttr(Attribute::OptimizeForSize),
           "Attributes 'optsize and optdebug' are incompatible!", V);
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 81d3243c887fce..0b6f738a58dca8 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -934,6 +934,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,
       case Attribute::NoUnwind:
       case Attribute::NoSanitizeBounds:
       case Attribute::NoSanitizeCoverage:
+      case Attribute::NoSanitizeRealtime:
       case Attribute::NullPointerIsValid:
       case Attribute::OptimizeForDebugging:
       case Attribute::OptForFuzzing:
diff --git a/llvm/test/Bitcode/attributes.ll b/llvm/test/Bitcode/attributes.ll
index 4402289ac170d9..835622276ef279 100644
--- a/llvm/test/Bitcode/attributes.ll
+++ b/llvm/test/Bitcode/attributes.ll
@@ -511,6 +511,12 @@ define void @f92() sanitize_realtime
         ret void;
 }
 
+; CHECK: define void @f93() #54
+define void @f93() nosanitize_realtime
+{
+        ret void;
+}
+
 ; CHECK: define void @f87() [[FNRETTHUNKEXTERN:#[0-9]+]]
 define void @f87() fn_ret_thunk_extern { ret void }
 
@@ -606,6 +612,7 @@ define void @initializes(ptr initializes((-4, 0), (4, 8)) %a) {
 ; CHECK: attributes #51 = { uwtable(sync) }
 ; CHECK: attributes #52 = { nosanitize_bounds }
 ; CHECK: attributes #53 = { sanitize_realtime }
+; CHECK: attributes #54 = { nosanitize_realtime }
 ; CHECK: attributes [[FNRETTHUNKEXTERN]] = { fn_ret_thunk_extern }
 ; CHECK: attributes [[SKIPPROFILE]] = { skipprofile }
 ; CHECK: attributes [[OPTDEBUG]] = { optdebug }
diff --git a/llvm/test/Bitcode/compatibility.ll b/llvm/test/Bitcode/compatibility.ll
index fd60c49a4be39b..c401cde8e146e7 100644
--- a/llvm/test/Bitcode/compatibility.ll
+++ b/llvm/test/Bitcode/compatibility.ll
@@ -1562,7 +1562,7 @@ exit:
   ; CHECK: select <2 x i1> <i1 true, i1 false>, <2 x i8> <i8 2, i8 3>, <2 x i8> <i8 3, i8 2>
 
   call void @f.nobuiltin() builtin
-  ; CHECK: call void @f.nobuiltin() #53
+  ; CHECK: call void @f.nobuiltin() #54
 
   call fastcc noalias ptr @f.noalias() noinline
   ; CHECK: call fastcc noalias ptr @f.noalias() #12
@@ -1992,6 +1992,9 @@ declare void @f.sanitize_numerical_stability() sanitize_numerical_stability
 declare void @f.sanitize_realtime() sanitize_realtime
 ; CHECK: declare void @f.sanitize_realtime() #52
 
+declare void @f.nosanitize_realtime() nosanitize_realtime
+; CHECK: declare void @f.nosanitize_realtime() #53
+
 ; CHECK: declare nofpclass(snan) float @nofpclass_snan(float nofpclass(snan))
 declare nofpclass(snan) float @nofpclass_snan(float nofpclass(snan))
 
@@ -2115,7 +2118,8 @@ define float @nofpclass_callsites(float %arg) {
 ; CHECK: attributes #50 = { allockind("alloc,uninitialized") }
 ; CHECK: attributes #51 = { sanitize_numerical_stability }
 ; CHECK: attributes #52 = { sanitize_realtime }
-; CHECK: attributes #53 = { builtin }
+; CHECK: attributes #53 = { nosanitize_realtime }
+; CHECK: attributes #54 = { builtin }
 
 ;; Metadata
 
diff --git a/llvm/test/Verifier/rtsan-attrs.ll b/llvm/test/Verifier/rtsan-attrs.ll
new file mode 100644
index 00000000000000..42ab85163642b1
--- /dev/null
+++ b/llvm/test/Verifier/rtsan-attrs.ll
@@ -0,0 +1,9 @@
+; RUN: not llvm-as -disable-output %s 2>&1 | FileCheck %s
+
+; CHECK: Attributes 'sanitize_realtime and nosanitize_realtime' are incompatible!
+; CHECK-NEXT: ptr @sanitize_nosanitize
+define void @sanitize_nosanitize() #0 {
+  ret void
+}
+
+attributes #0 = { sanitize_realtime nosanitize_realtime }

@llvmbot
Copy link
Member

llvmbot commented Aug 20, 2024

@llvm/pr-subscribers-llvm-ir

Author: Chris Apple (cjappl)

Changes

This new attribute should be incompatible with the sanitize_realtime attribute.


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

10 Files Affected:

  • (modified) llvm/docs/LangRef.rst (+5)
  • (modified) llvm/include/llvm/Bitcode/LLVMBitCodes.h (+1)
  • (modified) llvm/include/llvm/IR/Attributes.td (+3)
  • (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+2)
  • (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+2)
  • (modified) llvm/lib/IR/Verifier.cpp (+6)
  • (modified) llvm/lib/Transforms/Utils/CodeExtractor.cpp (+1)
  • (modified) llvm/test/Bitcode/attributes.ll (+7)
  • (modified) llvm/test/Bitcode/compatibility.ll (+6-2)
  • (added) llvm/test/Verifier/rtsan-attrs.ll (+9)
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 445980a18e7e93..4887ae6ee99668 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -2189,6 +2189,10 @@ example:
 ``nosanitize_coverage``
     This attribute indicates that SanitizerCoverage instrumentation is disabled
     for this function.
+``nosanitize_realtime``
+    This attribute indicates that the Realtime Sanitizer instrumentation is
+    disabled for this function.
+    This attribute is incompatible with the ``sanitize_realtime`` attribute.
 ``null_pointer_is_valid``
    If ``null_pointer_is_valid`` is set, then the ``null`` address
    in address-space 0 is considered to be a valid address for memory loads and
@@ -2315,6 +2319,7 @@ example:
     This attribute indicates that RealtimeSanitizer checks
     (realtime safety analysis - no allocations, syscalls or exceptions) are enabled
     for this function.
+    This attribute is incompatible with the ``nosanitize_realtime`` attribute.
 ``speculative_load_hardening``
     This attribute indicates that
     `Speculative Load Hardening <https://llvm.org/docs/SpeculativeLoadHardening.html>`_
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index 4beac37a583445..8a2e6583af87c5 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -759,6 +759,7 @@ enum AttributeKindCodes {
   ATTR_KIND_INITIALIZES = 94,
   ATTR_KIND_HYBRID_PATCHABLE = 95,
   ATTR_KIND_SANITIZE_REALTIME = 96,
+  ATTR_KIND_NO_SANITIZE_REALTIME = 97,
 };
 
 enum ComdatSelectionKindCodes {
diff --git a/llvm/include/llvm/IR/Attributes.td b/llvm/include/llvm/IR/Attributes.td
index 891e34fec0c798..80936c0ee83355 100644
--- a/llvm/include/llvm/IR/Attributes.td
+++ b/llvm/include/llvm/IR/Attributes.td
@@ -212,6 +212,9 @@ def NoSanitizeBounds : EnumAttr<"nosanitize_bounds", [FnAttr]>;
 /// No SanitizeCoverage instrumentation.
 def NoSanitizeCoverage : EnumAttr<"nosanitize_coverage", [FnAttr]>;
 
+/// No SanitizeRealtime instrumentation.
+def NoSanitizeRealtime : EnumAttr<"nosanitize_realtime", [FnAttr]>;
+
 /// Null pointer in address space zero is valid.
 def NullPointerIsValid : EnumAttr<"null_pointer_is_valid", [FnAttr]>;
 
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index d4dbab04e8ecdb..6767c7ca288bc4 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -2093,6 +2093,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
     return Attribute::NoSanitizeBounds;
   case bitc::ATTR_KIND_NO_SANITIZE_COVERAGE:
     return Attribute::NoSanitizeCoverage;
+  case bitc::ATTR_KIND_NO_SANITIZE_REALTIME:
+    return Attribute::NoSanitizeRealtime;
   case bitc::ATTR_KIND_NULL_POINTER_IS_VALID:
     return Attribute::NullPointerIsValid;
   case bitc::ATTR_KIND_OPTIMIZE_FOR_DEBUGGING:
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 03d0537291dada..de24dcdc27e266 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -795,6 +795,8 @@ static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
     return bitc::ATTR_KIND_NO_SANITIZE_BOUNDS;
   case Attribute::NoSanitizeCoverage:
     return bitc::ATTR_KIND_NO_SANITIZE_COVERAGE;
+  case llvm::Attribute::NoSanitizeRealtime:
+    return bitc::ATTR_KIND_NO_SANITIZE_REALTIME;
   case Attribute::NullPointerIsValid:
     return bitc::ATTR_KIND_NULL_POINTER_IS_VALID;
   case Attribute::OptimizeForDebugging:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index c095e47996ba17..e65ed7ee8b3f10 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2223,6 +2223,12 @@ void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
           "Attributes 'optdebug and optnone' are incompatible!", V);
   }
 
+  Check(!(Attrs.hasFnAttr(Attribute::SanitizeRealtime) &&
+          Attrs.hasFnAttr(Attribute::NoSanitizeRealtime)),
+        "Attributes "
+        "'sanitize_realtime and nosanitize_realtime' are incompatible!",
+        V);
+
   if (Attrs.hasFnAttr(Attribute::OptimizeForDebugging)) {
     Check(!Attrs.hasFnAttr(Attribute::OptimizeForSize),
           "Attributes 'optsize and optdebug' are incompatible!", V);
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 81d3243c887fce..0b6f738a58dca8 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -934,6 +934,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,
       case Attribute::NoUnwind:
       case Attribute::NoSanitizeBounds:
       case Attribute::NoSanitizeCoverage:
+      case Attribute::NoSanitizeRealtime:
       case Attribute::NullPointerIsValid:
       case Attribute::OptimizeForDebugging:
       case Attribute::OptForFuzzing:
diff --git a/llvm/test/Bitcode/attributes.ll b/llvm/test/Bitcode/attributes.ll
index 4402289ac170d9..835622276ef279 100644
--- a/llvm/test/Bitcode/attributes.ll
+++ b/llvm/test/Bitcode/attributes.ll
@@ -511,6 +511,12 @@ define void @f92() sanitize_realtime
         ret void;
 }
 
+; CHECK: define void @f93() #54
+define void @f93() nosanitize_realtime
+{
+        ret void;
+}
+
 ; CHECK: define void @f87() [[FNRETTHUNKEXTERN:#[0-9]+]]
 define void @f87() fn_ret_thunk_extern { ret void }
 
@@ -606,6 +612,7 @@ define void @initializes(ptr initializes((-4, 0), (4, 8)) %a) {
 ; CHECK: attributes #51 = { uwtable(sync) }
 ; CHECK: attributes #52 = { nosanitize_bounds }
 ; CHECK: attributes #53 = { sanitize_realtime }
+; CHECK: attributes #54 = { nosanitize_realtime }
 ; CHECK: attributes [[FNRETTHUNKEXTERN]] = { fn_ret_thunk_extern }
 ; CHECK: attributes [[SKIPPROFILE]] = { skipprofile }
 ; CHECK: attributes [[OPTDEBUG]] = { optdebug }
diff --git a/llvm/test/Bitcode/compatibility.ll b/llvm/test/Bitcode/compatibility.ll
index fd60c49a4be39b..c401cde8e146e7 100644
--- a/llvm/test/Bitcode/compatibility.ll
+++ b/llvm/test/Bitcode/compatibility.ll
@@ -1562,7 +1562,7 @@ exit:
   ; CHECK: select <2 x i1> <i1 true, i1 false>, <2 x i8> <i8 2, i8 3>, <2 x i8> <i8 3, i8 2>
 
   call void @f.nobuiltin() builtin
-  ; CHECK: call void @f.nobuiltin() #53
+  ; CHECK: call void @f.nobuiltin() #54
 
   call fastcc noalias ptr @f.noalias() noinline
   ; CHECK: call fastcc noalias ptr @f.noalias() #12
@@ -1992,6 +1992,9 @@ declare void @f.sanitize_numerical_stability() sanitize_numerical_stability
 declare void @f.sanitize_realtime() sanitize_realtime
 ; CHECK: declare void @f.sanitize_realtime() #52
 
+declare void @f.nosanitize_realtime() nosanitize_realtime
+; CHECK: declare void @f.nosanitize_realtime() #53
+
 ; CHECK: declare nofpclass(snan) float @nofpclass_snan(float nofpclass(snan))
 declare nofpclass(snan) float @nofpclass_snan(float nofpclass(snan))
 
@@ -2115,7 +2118,8 @@ define float @nofpclass_callsites(float %arg) {
 ; CHECK: attributes #50 = { allockind("alloc,uninitialized") }
 ; CHECK: attributes #51 = { sanitize_numerical_stability }
 ; CHECK: attributes #52 = { sanitize_realtime }
-; CHECK: attributes #53 = { builtin }
+; CHECK: attributes #53 = { nosanitize_realtime }
+; CHECK: attributes #54 = { builtin }
 
 ;; Metadata
 
diff --git a/llvm/test/Verifier/rtsan-attrs.ll b/llvm/test/Verifier/rtsan-attrs.ll
new file mode 100644
index 00000000000000..42ab85163642b1
--- /dev/null
+++ b/llvm/test/Verifier/rtsan-attrs.ll
@@ -0,0 +1,9 @@
+; RUN: not llvm-as -disable-output %s 2>&1 | FileCheck %s
+
+; CHECK: Attributes 'sanitize_realtime and nosanitize_realtime' are incompatible!
+; CHECK-NEXT: ptr @sanitize_nosanitize
+define void @sanitize_nosanitize() #0 {
+  ret void
+}
+
+attributes #0 = { sanitize_realtime nosanitize_realtime }

@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

(test failures looked unrelated, rebased to latest main to see if that resolved it).

Also, going to add a reviewer or two I missed in CODEOWNERS

@cjappl cjappl requested a review from pcc August 22, 2024 13:28
@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

Similarly unrelated build failure in CI:

�_bk;t=1724336701213�FAIL: lldb-shell :: Unwind/trap_frame_sym_ctx.test (1919 of 2590)

�_bk;t=1724336701213�******************** TEST 'lldb-shell :: Unwind/trap_frame_sym_ctx.test' FAILED ********************

�_bk;t=1724336701214�Exit Code: 1

�_bk;t=1724336701214�

�_bk;t=1724336701214�Command Output (stderr):


Some work being done on that now, based on this commit: 19d3f34

@pcc
Copy link
Contributor

pcc commented Aug 22, 2024

We usually only support __attribute__((no_sanitize("sanitizername"))) as the attribute for disabling a sanitizer. Some sanitizers have other attributes for disabling them but they only exist for backwards compatibility reasons.

@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

We usually only support __attribute__((no_sanitize("sanitizername"))) as the attribute for disabling a sanitizer. Some sanitizers have other attributes for disabling them but they only exist for backwards compatibility reasons.

Can you elaborate how one would access this in a transform pass of LLVM? I'm generally new to this area of the code, and thought that adding a nosanitize_x attribute was the only way to accomplish my goal.

The next PR after this one adds a check for this LLVM attribute in RealtimeSanitizer.cpp, and if this attribute exists on the function inserts a function call to bypass this in the Realtime Sanitizer stack. (That looks something like this). This seems plausible to do without the attribute as long as I have the correct thing to look for.

@pcc
Copy link
Contributor

pcc commented Aug 22, 2024

Apologies, for some reason I thought you were adding a Clang attribute. This seems fine then.

@pcc
Copy link
Contributor

pcc commented Aug 22, 2024

That being said, we normally only add positive LLVM attributes for sanitizers and arrange for the Clang attribute __attribute__((no_sanitize("sanitizername"))) to inhibit adding the LLVM attribute (see CodeGenFunction::StartFunction). Is there a reason why this approach doesn't work for the realtime sanitizer?

@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

That being said, we normally only add positive LLVM attributes for sanitizers and arrange for the Clang attribute __attribute__((no_sanitize("sanitizername"))) to inhibit adding the LLVM attribute (see CodeGenFunction::StartFunction). Is there a reason why this approach doesn't work for the realtime sanitizer?

This won't work in our case, because the realtime sanitizer is selective, ignoring the vast majority of functions, and only focusing on function calls from functions that have an ancestor marked [[clang::nonblocking]] (some more detail in our little design doc)

Because a user may want to ignore errors in a call stack that has a [[clang::nonblocking]] ancestor, we need to look for this LLVM attribute, and insert calls to bypass the stack (__rtsan_bypass_on/_off).

So:

  • Most functions - not attributed with either, will not be looked at by the realtime sanitizer or impacted at all
  • [[clang::nonblocking]] functions - in our transform will add the __rtsan_realtime_enter/_exit commands
  • __attribute__((no_sanitize("sanitizername"))) called from a [[clang::noblocking]] setting - need to bypass rtsan with bypass_on/_off.

Because we care about these function in this third state, we can't just omit the sanitizer, we have to add the attribute and transform them in our pass.

I guess another way to do it would be add the _bypass to all functions, and only add the _realtime_enter to functions marked [[clang::nonblocking]], but that seems like it would add runtime overhead to user code as compared to this approach

@pcc
Copy link
Contributor

pcc commented Aug 22, 2024

So do you really need the LLVM attributes then? Would it not suffice to have Clang IRGen add the calls to __rtsan_bypass_on/off and __rtsan_realtime_enter/exit based on the Clang attributes? The other sanitizers have passes because they need to be able to operate on arbitrary instructions in each function after optimization, but if RTSan only needs to affect function entry/exit then that seems much more straightforwardly accomplished in the frontend.

@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

So do you really need the LLVM attributes then? Would it not suffice to have Clang IRGen add the calls to __rtsan_bypass_on/off and __rtsan_realtime_enter/exit based on the Clang attributes? The other sanitizers have passes because they need to be able to operate on arbitrary instructions in each function after optimization, but if RTSan only needs to affect function entry/exit then that seems much more straightforwardly accomplished in the frontend.

We discussed this a bit in the RFC thread (apologies in advance for the size of it, it became a monster), this comment and below:
https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837/104?u=cjappl

The reasoning we came up with to have it be in LLVM directly is in this comment:
https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837/107?u=cjappl

@pcc
Copy link
Contributor

pcc commented Aug 22, 2024

Thanks, I didn't see that discussion.

Re 2: this can be accomplished via an API, no? You can provide functions such as void rtsanMarkAsNonblocking(Function *F); and void rtsanMarkAsBypass(Function *F); and clang and other frontends can call it.

Re 3: we can always defer adding the attribute until/unless the planned functionality is actually implemented.

But I don't have a strong opinion. If @vitalybuka is fine with this being LLVM attributes it's fine with me as well.

@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

Thanks a lot for the feedback, we are new around these parts and just getting our bearings. We really appreciate having review and feedback from folks who have more context than us! I'll wait to hear if Vitaly has any additional feedback that wasn't given on the thread.

Re 2: this can be accomplished via an API, no? You can provide functions such as void rtsanMarkAsNonblocking(Function *F); and void rtsanMarkAsBypass(Function *F); and clang and other frontends can call it.

Where would such a function live? Can you point me in the direction of other similar functions?

@cjappl
Copy link
Contributor Author

cjappl commented Aug 22, 2024

I also just recalled it was Vitaly that set us on the path of LLVM to begin with, see the discussion here:

#100192 (comment)

@pcc
Copy link
Contributor

pcc commented Aug 22, 2024

Where would such a function live? Can you point me in the direction of other similar functions?

Functions like this would normally be under llvm/lib/Transforms/Utils. You can look at existing inclusions of headers in llvm/Transforms/Utils under clang/lib/CodeGen.

@pcc
Copy link
Contributor

pcc commented Aug 23, 2024

Re 3: we can always defer adding the attribute until/unless the planned functionality is actually implemented.

Another point regarding this: when adding new functionality to sanitizers we often need to be able to opt out the new functionality in existing code in order to avoid breaking existing usage of the sanitizer. So in the end you might end up needing nonblocking function checks to be controlled separately from loop checks (or any other checks that you may implement in the future). That would likely imply a separate LLVM attribute specifically for loop checks, rendering the LLVM attributes for nonblocking function checks unnecessary.

@cjappl
Copy link
Contributor Author

cjappl commented Aug 23, 2024

These all seem like totally reasonable suggestions. I'll wait for Vitaly to chime in with his opinion! I have limited expertise in this area, so I will go with what seasoned hands think are best :)

If there is a split or a "either way" I would vote for keeping it how it is now, but if there is something that tilts it to the "just clang" side, I am happy to go back and revise the work already done, strip out the attributes etc.

Thanks again for your thoughts on this @pcc. Really appreciate it.

@cjappl
Copy link
Contributor Author

cjappl commented Aug 23, 2024

Build is failing for same reason as yesterday, still unrelated:

�_bk;t=1724431385857�FAIL: lldb-shell :: Unwind/trap_frame_sym_ctx.test (1944 of 2593)

�_bk;t=1724431385857�******************** TEST 'lldb-shell :: Unwind/trap_frame_sym_ctx.test' FAILED ********************

�_bk;t=1724431385858�Exit Code: 1

�_bk;t=1724431385858�

�_bk;t=1724431385858�Command Output (stderr):

�_bk;t=1724431385858�--


https://buildkite.com/llvm-project/github-pull-requests/builds/94386#0191800e-c66f-424b-9029-34abc10006d8

@cjappl
Copy link
Contributor Author

cjappl commented Aug 26, 2024

@pcc - I think I'll submit this as is tomorrow morning PST following vitaly's approval. If you want to continue talking it over, I could I file a discourse thread and ping the interested parties (vitaly, nikic, you, me, people from the original RFC thread) and we can hash out if one approach is better than the other. If doing it in the frontend is best, I can submit a patch reverting any of the LLVM and putting it in clang in one swoop to maintain the functionality we have built so far.

If you don't feel strongly, I can just keep trucking with this approach! I will add you to the transform review (following this) if you're amenable :)

@cjappl cjappl merged commit 178fc47 into llvm:main Aug 26, 2024
9 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 26, 2024

LLVM Buildbot has detected a new failure on builder clang-hip-vega20 running on hip-vega20-0 while building llvm at step 3 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/4312

Here is the relevant piece of the build log for the reference
Step 3 (annotate) failure: '../llvm-zorg/zorg/buildbot/builders/annotated/hip-build.sh --jobs=' (failure)
...
[38/40] : && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -O3 -DNDEBUG  External/HIP/CMakeFiles/memmove-hip-6.0.2.dir/memmove.hip.o -o External/HIP/memmove-hip-6.0.2  --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --hip-link -rtlib=compiler-rt -unwindlib=libgcc -frtlib-add-rpath && cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /usr/local/bin/cmake -E create_symlink /buildbot/llvm-test-suite/External/HIP/memmove.reference_output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/memmove.reference_output-hip-6.0.2
[39/40] /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -DNDEBUG  -O3 -DNDEBUG   -w -Werror=date-time --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --offload-arch=gfx908 --offload-arch=gfx90a --offload-arch=gfx1030 --offload-arch=gfx1100 -xhip -mfma -MD -MT External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -MF External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o.d -o External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -c /buildbot/llvm-test-suite/External/HIP/workload/ray-tracing/TheNextWeek/main.cc
[40/40] : && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -O3 -DNDEBUG  External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -o External/HIP/TheNextWeek-hip-6.0.2  --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --hip-link -rtlib=compiler-rt -unwindlib=libgcc -frtlib-add-rpath && cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /usr/local/bin/cmake -E create_symlink /buildbot/llvm-test-suite/External/HIP/TheNextWeek.reference_output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/TheNextWeek.reference_output-hip-6.0.2
+ build_step 'Testing HIP test-suite'
+ echo '@@@BUILD_STEP Testing HIP test-suite@@@'
@@@BUILD_STEP Testing HIP test-suite@@@
+ ninja -v check-hip-simple
[0/1] cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
-- Testing: 7 tests, 7 workers --
Testing:  0.. 10.. 20.. 30.. 40
FAIL: test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test (4 of 7)
******************** TEST 'test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test' FAILED ********************

/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --append-exitstatus --redirect-output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out --redirect-input /dev/null --summary /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.time /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend-hip-6.0.2
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP ; /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2

+ cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP
+ /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2
/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target: Comparison failed, textual difference between 'M' and 'i'

********************
/usr/bin/strip: /bin/bash.stripped: Bad file descriptor
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test


Testing Time: 368.45s

Total Discovered Tests: 7
  Passed: 6 (85.71%)
  Failed: 1 (14.29%)
FAILED: External/HIP/CMakeFiles/check-hip-simple-hip-6.0.2 
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
ninja: build stopped: subcommand failed.
Step 12 (Testing HIP test-suite) failure: Testing HIP test-suite (failure)
@@@BUILD_STEP Testing HIP test-suite@@@
+ ninja -v check-hip-simple
[0/1] cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
-- Testing: 7 tests, 7 workers --
Testing:  0.. 10.. 20.. 30.. 40
FAIL: test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test (4 of 7)
******************** TEST 'test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test' FAILED ********************

/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --append-exitstatus --redirect-output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out --redirect-input /dev/null --summary /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.time /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend-hip-6.0.2
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP ; /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2

+ cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP
+ /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2
/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target: Comparison failed, textual difference between 'M' and 'i'

********************
/usr/bin/strip: /bin/bash.stripped: Bad file descriptor
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test


Testing Time: 368.45s

Total Discovered Tests: 7
  Passed: 6 (85.71%)
  Failed: 1 (14.29%)
FAILED: External/HIP/CMakeFiles/check-hip-simple-hip-6.0.2 
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test memmove-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
ninja: build stopped: subcommand failed.
program finished with exit code 1
elapsedTime=988.028483

@cjappl cjappl deleted the nosanitize_attr branch August 30, 2024 14:16
@cjappl
Copy link
Contributor Author

cjappl commented Aug 30, 2024

After discussion on #106125 I am going to revert this in favor of doing it the "lsan way" which introduces a scoped disabler, for more details on that check out #106736 .

I will request a review on the reversion in a few

cjappl added a commit to cjappl/llvm-project that referenced this pull request Aug 30, 2024
cjappl added a commit that referenced this pull request Aug 30, 2024
…" (#106743)

This reverts commit 178fc47.

This attribute was not needed now that we are using the lsan style
ScopedDisabler for disabling this sanitizer

See #106736 
#106125 

For more discussion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants