Skip to content

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