Skip to content

[FuncAttrs] Don't infer noundef for functions with sanitize_memory attribute #76691

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Jan 1, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Jan 1, 2024

MemorySanitizer assumes that the definition and declaration of a function will be consistent. If we add noundef for some definitions, it will break msan.

Fix buildbot failure caused by #76553.

@llvmbot
Copy link
Member

llvmbot commented Jan 1, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

MemorySanitizer assumes that the definition and declaration of a function will be consistent. If we add noundef for some definitions, it will break msan.

Fix buildbot failure caused by #76553.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/FunctionAttrs.cpp (+6)
  • (modified) llvm/test/Transforms/FunctionAttrs/noundef.ll (+9)
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index ce083979afc63a..7ebf265e17ba1f 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -1296,6 +1296,12 @@ static void addNoUndefAttrs(const SCCNodeSet &SCCNodes,
     if (!F->hasExactDefinition())
       return;
 
+    // MemorySanitizer assumes that the definition and declaration of a
+    // function will be consistent. A function with sanitize_memory attribute
+    // should be skipped from inference.
+    if (F->hasFnAttribute(Attribute::SanitizeMemory))
+      continue;
+
     if (F->getReturnType()->isVoidTy())
       continue;
 
diff --git a/llvm/test/Transforms/FunctionAttrs/noundef.ll b/llvm/test/Transforms/FunctionAttrs/noundef.ll
index b357587cc12394..946b562f39553e 100644
--- a/llvm/test/Transforms/FunctionAttrs/noundef.ll
+++ b/llvm/test/Transforms/FunctionAttrs/noundef.ll
@@ -143,3 +143,12 @@ define i32 @test_noundef_prop() {
   %ret = call i32 @test_ret_constant()
   ret i32 %ret
 }
+
+; Don't deduce noundef for functions with sanitize_memory.
+define i32 @test_ret_constant_msan() sanitize_memory {
+; CHECK-LABEL: define i32 @test_ret_constant_msan(
+; CHECK-SAME: ) #[[ATTR1:[0-9]+]] {
+; CHECK-NEXT:    ret i32 0
+;
+  ret i32 0
+}

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -1296,6 +1296,12 @@ static void addNoUndefAttrs(const SCCNodeSet &SCCNodes,
if (!F->hasExactDefinition())
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but I think this should be continue rather than return as well.

@dtcxzyw dtcxzyw merged commit 7e405eb into llvm:main Jan 1, 2024
@dtcxzyw dtcxzyw deleted the fix-msan-noundef branch January 1, 2024 22:59
@vitalybuka
Copy link
Collaborator

Thank you!

Probably we can skip only not internal/private linkage, but I don't think performance improvement will be meaningful for msan, So as is LGTM for simplicity.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants