Skip to content

[StackSafetyAnalysis] Bail out if MemIntrinsic length is -1 #77837

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jan 11, 2024

Clang generates llvm.memset.p0.i64 with a length of -1 for the following code in
-stdlib=libc++ -std=c++20 mode
(#77210 (comment))

bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}

Sizes = [0xffffffffffffffff, 0). SizeRange = [0, 0-1), leading to
assert(!isUnsafe(SizeRange)); failure. Bail out if the length is -1.
Other negative values are handled by the existing condition.

Clang generates llvm.memset.p0.i64 with size of -1 for following code in
`-stdlib=libc++ -std=c++20` mode
(llvm#77210 (comment))
```cpp
bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}
```

`Sizes = [0xffffffffffffffff, 0)`. `SizeRange` is `[0, 0-1)`, leading to
`assert(!isUnsafe(SizeRange));` failure. Bail out if memset size is -1.
@MaskRay MaskRay requested a review from vitalybuka January 11, 2024 21:32
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jan 11, 2024
@MaskRay MaskRay requested a review from kstoimenov January 11, 2024 21:32
@llvmbot
Copy link
Member

llvmbot commented Jan 11, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Fangrui Song (MaskRay)

Changes

Clang generates llvm.memset.p0.i64 with size of -1 for following code in
-stdlib=libc++ -std=c++20 mode
(#77210 (comment))

bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}

Sizes = [0xffffffffffffffff, 0). SizeRange is [0, 0-1), leading to
assert(!isUnsafe(SizeRange)); failure. Bail out if memset size is -1.


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/StackSafetyAnalysis.cpp (+1-1)
  • (modified) llvm/test/Analysis/StackSafetyAnalysis/memintrin.ll (+34)
diff --git a/llvm/lib/Analysis/StackSafetyAnalysis.cpp b/llvm/lib/Analysis/StackSafetyAnalysis.cpp
index da21e3f28e7899..19991c1a7baee6 100644
--- a/llvm/lib/Analysis/StackSafetyAnalysis.cpp
+++ b/llvm/lib/Analysis/StackSafetyAnalysis.cpp
@@ -331,7 +331,7 @@ ConstantRange StackSafetyLocalAnalysis::getMemIntrinsicAccessRange(
   const SCEV *Expr =
       SE.getTruncateOrZeroExtend(SE.getSCEV(MI->getLength()), CalculationTy);
   ConstantRange Sizes = SE.getSignedRange(Expr);
-  if (Sizes.getUpper().isNegative() || isUnsafe(Sizes))
+  if (!Sizes.getUpper().isStrictlyPositive() || isUnsafe(Sizes))
     return UnknownRange;
   Sizes = Sizes.sextOrTrunc(PointerSize);
   ConstantRange SizeRange(APInt::getZero(PointerSize), Sizes.getUpper() - 1);
diff --git a/llvm/test/Analysis/StackSafetyAnalysis/memintrin.ll b/llvm/test/Analysis/StackSafetyAnalysis/memintrin.ll
index 791fb35ce2b759..6913816cc8fe7a 100644
--- a/llvm/test/Analysis/StackSafetyAnalysis/memintrin.ll
+++ b/llvm/test/Analysis/StackSafetyAnalysis/memintrin.ll
@@ -98,6 +98,40 @@ entry:
   ret void
 }
 
+define void @MemsetHugeUpper_m1(i1 %bool) {
+; CHECK-LABEL: MemsetHugeUpper_m1 dso_preemptable{{$}}
+; CHECK-NEXT: args uses:
+; CHECK-NEXT: allocas uses:
+; CHECK-NEXT:   x[4]: full-set
+entry:
+  %x = alloca i32, align 4
+  br i1 %bool, label %if.then, label %if.end
+
+if.then:
+  call void @llvm.memset.p0.i64(ptr %x, i8 0, i64 -1, i1 false)
+  br label %if.end
+
+if.end:
+  ret void
+}
+
+define void @MemsetHugeUpper_m2(i1 %bool) {
+; CHECK-LABEL: MemsetHugeUpper_m2 dso_preemptable{{$}}
+; CHECK-NEXT: args uses:
+; CHECK-NEXT: allocas uses:
+; CHECK-NEXT:   x[4]: full-set
+entry:
+  %x = alloca i32, align 4
+  br i1 %bool, label %if.then, label %if.end
+
+if.then:
+  call void @llvm.memset.p0.i64(ptr %x, i8 0, i64 -2, i1 false)
+  br label %if.end
+
+if.end:
+  ret void
+}
+
 define void @MemcpyInBounds() {
 ; CHECK-LABEL: MemcpyInBounds dso_preemptable{{$}}
 ; CHECK-NEXT: args uses:

@MaskRay MaskRay changed the title [StackSafetyAnalysis] Bail out if memset size is -1 [StackSafetyAnalysis] Bail out if MemIntrinsic length is -1 Jan 11, 2024
@MaskRay MaskRay merged commit a6d4017 into llvm:main Jan 11, 2024
@MaskRay MaskRay deleted the stacksafety-memset branch January 11, 2024 21:54
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
Clang generates llvm.memset.p0.i64 with a length of -1 for the following
code in
`-stdlib=libc++ -std=c++20` mode

(llvm#77210 (comment))
```cpp
bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}
```

`Sizes = [0xffffffffffffffff, 0)`. `SizeRange = [0, 0-1)`, leading to
`assert(!isUnsafe(SizeRange));` failure. Bail out if the length is -1.
Other negative values are handled by the existing condition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants