[SimplifyLibCalls] avoid slicing 64-bit integers in an ILP32 build (PR #54739)

[msebor]

Summary

Passing a 64-bit result of a call like Value::getZExtValue() as an argument to StringRef::substr() in a 32-bit build truncates the value and, in the two test cases in bug 54739, leads to incorrect folding. The attached change adjusts the calls to StringRef::substr() to avoid the truncation in these two instances.

I considered changing StringRef::substr() to use uint64_t but since the use of size_t in the StringRef API is pervasive and due to its interplay with StringRef::npos and sometimes std::string::npos and the potential for unintended semantic changes, decided against it. I reviewed the rest of SimplifyLibCalls.cpp for similar problems and didn't spot any, but I suspect there may be others in the rest of the code base.

Tested on x86_64-linux in ILP32 and LP64 modes. (The ILP32 test run shows a few crashes that appear unrelated to this change.)

[xbolva00]

ILP32 test run shows a few crashes that appear unrelated to this change

Is this even supported? I am wondering whether it is worth to fix it at all (and make code worse to read) if there are more crashes anyway.

[nikic]

If we don't want to change the StringRef API, possibly add a common helper to this file?

static StringRef substr(StringRef Str, uint64_t Len) {
  // Avoid truncating the length if size_t is 32-bits.
  return Len >= Str.size() ? Str : Str.substr(0, Len);
}

[msebor]

In D123472#3441702, @xbolva00 wrote:

ILP32 test run shows a few crashes that appear unrelated to this change

Is this even supported? I am wondering whether it is worth to fix it at all (and make code worse to read) if there are more crashes anyway.

I opened PR 54129 with more details. It would be good to know if building 64-bit programs with a 32-bit Clang/LLVM isn't meant to work (if it isn't, adding an error to point that out would be helpful).

[efriedma]

In D123472#3441702, @xbolva00 wrote:

ILP32 test run shows a few crashes that appear unrelated to this change

Is this even supported? I am wondering whether it is worth to fix it at all (and make code worse to read) if there are more crashes anyway.

It's one of those things where its not intentionally unsupported, but gets very little testing these days. There's very little reason to run 32-bit LLVM on a 64-bit host, and most cross-compiles are going to be on 64-bit hosts.

In D123472#3442229, @nikic wrote:

If we don't want to change the StringRef API, possibly add a common helper to this file?

Agree.

[msebor]

This revision adds the suggested substr helper function.

[nikic]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

207

This comment is redundant with the function comment now.

463

We shouldn't need both the std::min and the helper function.

[msebor]

Anything else?

[nikic]

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

static Value *copyFlags(const CallInst &Old, Value *New) {
   if (auto *NewCI = dyn_cast_or_null<CallInst>(New))
     NewCI->setTailCallKind(Old.getTailCallKind());
   return New;
 }
 
 //===----------------------------------------------------------------------===//
 // String and Memory Library Call Optimizations
 //===----------------------------------------------------------------------===//
 
 Value *LibCallSimplifier::optimizeStrCat(CallInst *CI, IRBuilderBase &B) {
   // Extract some information from the instruction
   Value *Dst = CI->getArgOperand(0);
   Value *Src = CI->getArgOperand(1);
   annotateNonNullNoUndefBasedOnAccess(CI, {0, 1});
 
   // See if we can get the length of the input string.
   uint64_t Len = GetStringLength(Src);
if (Length == 1) // strncmp(x,y,1) -> memcmp(x,y,1)
     return copyFlags(*CI, emitMemCmp(Str1P, Str2P, Size, B, DL, TLI));
 
   StringRef Str1, Str2;
   bool HasStr1 = getConstantStringInfo(Str1P, Str1);
   bool HasStr2 = getConstantStringInfo(Str2P, Str2);
 
   // strncmp(x, y)  -> cnst  (if both x and y are constant strings)
   if (HasStr1 && HasStr2) {
-    StringRef SubStr1 = Str1.substr(0, Length);
-    StringRef SubStr2 = Str2.substr(0, Length);
+    // Avoid truncating the 64-bit Length to 32 bits in ILP32.
+    StringRef::size_type MinLen1 = std::min((uint64_t)Str1.size(), Length);
+    StringRef::size_type MinLen2 = std::min((uint64_t)Str2.size(), Length);
+    StringRef SubStr1 = Str1.substr(0, MinLen1);
+    StringRef SubStr2 = Str2.substr(0, MinLen2);
     return ConstantInt::get(CI->getType(), SubStr1.compare(SubStr2));
   }
 
   if (HasStr1 && Str1.empty()) // strncmp("", x, n) -> -*x
     return B.CreateNeg(B.CreateZExt(
         B.CreateLoad(B.getInt8Ty(), Str2P, "strcmpload"), CI->getType()));
 
   if (HasStr2 && Str2.empty()) // strncmp(x, "", n) -> *x
     return B.CreateZExt(B.CreateLoad(B.getInt8Ty(), Str1P, "strcmpload"),
                         CI->getType());
 
   uint64_t Len1 = GetStringLength(Str1P);
   if (Len1)
Value *SrcPlus =
         B.CreateGEP(B.getInt8Ty(), SrcStr, B.getInt64(Pos), "memchr.ptr");
     return B.CreateSelect(Cmp, NullPtr, SrcPlus);
   }
 
   if (!LenC)
     // From now on we need a constant length and constant array.
     return nullptr;
 
-  // Truncate the string to LenC.
-  Str = Str.substr(0, LenC->getZExtValue());
+  // Truncate the string to LenC without slicing when targeting LP64
+  // on an ILP32 host.
+  uint64_t EndOff = std::min(LenC->getZExtValue(), (uint64_t)StringRef::npos);
+  Str = Str.substr(0, EndOff);
 
   // If the char is variable but the input str and length are not we can turn
   // this memchr call into a simple bit field test. Of course this only works
   // when the return value is only checked against null.
   //
   // It would be really nice to reuse switch lowering here but we can't change
   // the CFG at this point.
   //

llvm/test/Transforms/InstCombine/memchr-4.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+; Verify that an excessive size to memchr() isn't truncated to an in-bounds
+; value that results in the call being incorrectly folded (as might happen
+; when LLVM is compiled in ILP32 mode).
+
+declare i8* @memchr(i8*, i32, i64)
+
+@ax = external global [0 x i8]
+@a12345 = constant [5 x i8] c"\01\02\03\04\05"
+
+
+; Do not fold memchr(ax, 1, UINT_MAX + (size_t)1) to null.  Only the first
+; byte in ax must be dereferenceable.
+
+define i8* @call_memchr_ax_2_uimax_p1() {
+; CHECK-LABEL: @call_memchr_ax_2_uimax_p1(
+; CHECK-NEXT:    [[RES:%.*]] = call i8* @memchr(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([0 x i8], [0 x i8]* @ax, i64 0, i64 0), i32 1, i64 4294967296)
+; CHECK-NEXT:    ret i8* [[RES]]
+;
+
+  %ptr = getelementptr [0 x i8], [0 x i8]* @ax, i32 0, i32 0
+  %res = call i8* @memchr(i8* %ptr, i32 1, i64 4294967296)
+  ret i8* %res
+}
+
+
+; Do not fold memchr(ax, 1, UINT_MAX + (size_t)2) to *ax == 1 ? ax : null.
+; As above, only the first byte in ax must be dereferenceable.
+
+define i8* @call_memchr_ax_2_uimax_p2() {
+; CHECK-LABEL: @call_memchr_ax_2_uimax_p2(
+; CHECK-NEXT:    [[RES:%.*]] = call i8* @memchr(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([0 x i8], [0 x i8]* @ax, i64 0, i64 0), i32 1, i64 4294967296)
+; CHECK-NEXT:    ret i8* [[RES]]
+;
+
+  %ptr = getelementptr [0 x i8], [0 x i8]* @ax, i32 0, i32 0
+  %res = call i8* @memchr(i8* %ptr, i32 1, i64 4294967296)
+  ret i8* %res
+}
+
+
+; Fold memchr(a12345, 3, UINT_MAX + (size_t)2) to a12345 + 2 (and not to
+; null).
+
+define i8* @fold_memchr_a12345_3_uimax_p2() {
+; CHECK-LABEL: @fold_memchr_a12345_3_uimax_p2(
+; CHECK-NEXT:    ret i8* getelementptr inbounds ([5 x i8], [5 x i8]* @a12345, i64 0, i64 2)
+;
+
+  %ptr = getelementptr [5 x i8], [5 x i8]* @a12345, i32 0, i32 0
+  %res = call i8* @memchr(i8* %ptr, i32 3, i64 4294967297)
+  ret i8* %res
+}
+
+
+; Do not fold memchr(a12345, c, UINT_MAX + (size_t)2).
+
+define i8* @fold_memchr_a12345_c_uimax_p2(i32 %0) {
+; CHECK-LABEL: @fold_memchr_a12345_c_uimax_p2(
+; CHECK-NEXT:    [[RES:%.*]] = call i8* @memchr(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([5 x i8], [5 x i8]* @a12345, i64 0, i64 0), i32 [[TMP0:%.*]], i64 4294967297)
+; CHECK-NEXT:    ret i8* [[RES]]
+;
+
+  %ptr = getelementptr [5 x i8], [5 x i8]* @a12345, i32 0, i32 0
+  %res = call i8* @memchr(i8* %ptr, i32 %0, i64 4294967297)
+  ret i8* %res
+}

llvm/test/Transforms/InstCombine/strncmp-3.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+; Verify that an excessive size to strncmp() isn't truncated to an in-bounds
+; value that results in the call being incorrectly folded (as might happen
+; when LLVM is compiled in ILP32 mode).
+
+declare i32 @strncmp(i8*, i8*, i64)
+
+@ax = external global [0 x i8]
+@bx = external global [0 x i8]
+
+@a12345 = constant [5 x i8] c"\01\02\03\04\05"
+@a123456 = constant [6 x i8] c"\01\02\03\04\05\06"
+
+
+; Do not fold strncmp(ax, bx, UINT_MAX + (size_t)1) to 0.
+
+define i32 @call_strncmp_ax_bx_uimax_p1() {
+; CHECK-LABEL: @call_strncmp_ax_bx_uimax_p1(
+; CHECK-NEXT:    [[RES:%.*]] = call i32 @strncmp(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([0 x i8], [0 x i8]* @ax, i64 0, i64 0), i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([0 x i8], [0 x i8]* @bx, i64 0, i64 0), i64 4294967296)
+; CHECK-NEXT:    ret i32 [[RES]]
+;
+
+  %p1 = getelementptr [0 x i8], [0 x i8]* @ax, i32 0, i32 0
+  %p2 = getelementptr [0 x i8], [0 x i8]* @bx, i32 0, i32 0
+  %res = call i32 @strncmp(i8* %p1, i8* %p2, i64 4294967296)
+  ret i32 %res
+}
+
+
+; Do not fold strncmp(ax, bx, UINT_MAX + (size_t)2) to *ax - *bx.
+
+define i32 @call_strncmp_ax_bx_uimax_p2() {
+; CHECK-LABEL: @call_strncmp_ax_bx_uimax_p2(
+; CHECK-NEXT:    [[RES:%.*]] = call i32 @strncmp(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([0 x i8], [0 x i8]* @ax, i64 0, i64 0), i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([0 x i8], [0 x i8]* @bx, i64 0, i64 0), i64 4294967296)
+; CHECK-NEXT:    ret i32 [[RES]]
+;
+
+  %p1 = getelementptr [0 x i8], [0 x i8]* @ax, i32 0, i32 0
+  %p2 = getelementptr [0 x i8], [0 x i8]* @bx, i32 0, i32 0
+  %res = call i32 @strncmp(i8* %p1, i8* %p2, i64 4294967296)
+  ret i32 %res
+}
+
+
+; Fold strncmp(a12345, a123456, UINT_MAX + (size_t)2) to -1 (and not to 0).
+
+define i32 @fold_strncmp_a12345_2_uimax_p2() {
+; CHECK-LABEL: @fold_strncmp_a12345_2_uimax_p2(
+; CHECK-NEXT:    ret i32 -1
+;
+
+  %p1 = getelementptr [5 x i8], [5 x i8]* @a12345, i32 0, i32 0
+  %p2 = getelementptr [6 x i8], [6 x i8]* @a123456, i32 0, i32 0
+  %res = call i32 @strncmp(i8* %p1, i8* %p2, i64 4294967297)
+  ret i32 %res
+}
+
+
+; Fold strncmp(a123456, a12345, UINT_MAX + (size_t)3) to +1 (and not to 0).
+
+define i32 @fold_strncmp_a12345_2_uimax_p3() {
+; CHECK-LABEL: @fold_strncmp_a12345_2_uimax_p3(
+; CHECK-NEXT:    ret i32 1
+;
+
+  %p1 = getelementptr [6 x i8], [6 x i8]* @a123456, i32 0, i32 0
+  %p2 = getelementptr [5 x i8], [5 x i8]* @a12345, i32 0, i32 0
+  %res = call i32 @strncmp(i8* %p1, i8* %p2, i64 4294967298)
+  ret i32 %res
+}