[InstCombine] Adjust snprintf folding of constant strings (PR #56598)

[msebor]

Summary

The snprintf library call handler doesn't consider the POSIX requirement that the function fail and set errno = EOVERFLOW when the second argument, n, is greater than INT_MAX, and folds the subset of such calls to the function with strings of known length to constants. At the same time the handler only folds these calls if the second argument is greater than the length of the string, and avoids those where it would imply truncation.

This change adjusts the handler to fold all calls with constant strings up to n == INT_MAX but not others. This lets the calls that are required to fail according to POSIX fail on conforming implementations, while letting the folder emit efficient code for the truncating subset of the calls.

[majnemer]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2846

Does this do the right thing for targets whose INT_MAX is 16-bit?

[msebor]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2846

This function isn't executed for such targets because it's gated by TargetLibraryInfoImpl::isValidProtoForLibFunc which expects int to have 32 bits. This may be an okay assumption to make in this case since (if) most 16-bit targets are bare metal these days, but it's not an unavoidable one (not to mention it's not likely to be intended). I only last week (in D129915) discovered the relatively recently introduced getIntSize() function (added via D99438). If/when the part of D129915 that lifts the 32-bit assumption is accepted, this function could also be used here and in the rest of the code that makes it. I'm happy to do that in a follow-up.

[majnemer]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

2846

Sounds good!

[efriedma]

There are two separate conditions noted in POSIX related to EOVERFLOW (https://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html):

• The value to be returned is greater than {INT_MAX}
• The value of n is greater than {INT_MAX}

To be clear, this is intended to address the case of the value of "n", but not the case of the value to be returned? (I guess it would be a bit of a pain to write a testcase.)

[msebor]

I think I just equated the 32-bit StringRef limit with INT_MAX. Thanks for pointing that out!

While re-reviewing the function I noticed a couple more instances of the same problem that I had missed. Rather than duplicating the fix in all three places I factored it out into a new helper and added more tests. I also removed the 32-bit int assumption noted by @majnemer (it's not exercised until TargetLibraryInfoImpl::isValidProtoForLibFunc is adjusted as I explained).

As for testing the handling of excessive output, I can't think of a way without enhancing the function to handle more complex format strings, including directives with a width. Then we could use snprintf(0, 0, "x%*s", INT_MAX, "") to exceed the limit without hardcoding long strings. It's probably not worth the complexity just to test this unlikely case but supporting more involved format strings with multiple %s directives would make it possible to transform the idiom of using snprintf to concatenate strings, like snprintf(buf, sizeof buf, "%s/%s", dirname, filename), and supporting precision (and width) is simple. Another improvement to make this generally useful is to handle nonconstant strings allocated in arrays of known size. But that's for another day.

[msebor]

Ping: @efriedma, do you (or anyone else) have any other questions/comments or is everyone happy with the last revision?

[efriedma]

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

 
 Value *LibCallSimplifier::optimizeSnPrintFString(CallInst *CI,
                                                  IRBuilderBase &B) {
   // Check for size
   ConstantInt *Size = dyn_cast<ConstantInt>(CI->getArgOperand(1));
   if (!Size)
     return nullptr;
 
+  Value *DstArg = CI->getArgOperand(0);
+  Value *FmtArg = CI->getArgOperand(2);
+
   uint64_t N = Size->getZExtValue();
   // Check for a fixed format string.
   StringRef FormatStr;
-  if (!getConstantStringInfo(CI->getArgOperand(2), FormatStr))
+  if (!getConstantStringInfo(FmtArg, FormatStr))
     return nullptr;
 
   // If we just have a format string (nothing else crazy) transform it.
   if (CI->arg_size() == 3) {
     // Make sure there's no % in the constant array.  We could try to handle
     // %% -> % in the future if we cared.
-    if (FormatStr.contains('%'))
-      return nullptr; // we found a format specifier, bail out.
+    if (FormatStr.contains('%') || N > (uint64_t)maxIntN(32))
+      // Bail if the format string contains a formatting directive or
+      // if the bound exceeds INT_MAX.  POSIX requires implementations
+      // to set errno to EOVERFLOW for the latter.
+      return nullptr;
 
     if (N == 0)
       return ConstantInt::get(CI->getType(), FormatStr.size());
-    else if (N < FormatStr.size() + 1)
-      return nullptr;
 
-    // snprintf(dst, size, fmt) -> llvm.memcpy(align 1 dst, align 1 fmt,
-    // strlen(fmt)+1)
+    if (N >= FormatStr.size())
+      // Copy the full format string, including the terminating nul (which
+      // must be present regardless of the bound).
+      N = FormatStr.size() + 1;
+
+    // Transform snprintf(dst, N, fmt) to lvm.memcpy(dst, fmt, N') with
+    // N' <= strlen(dst) + 1.
     copyFlags(
         *CI,
         B.CreateMemCpy(
-            CI->getArgOperand(0), Align(1), CI->getArgOperand(2), Align(1),
-            ConstantInt::get(DL.getIntPtrType(CI->getContext()),
-                             FormatStr.size() + 1))); // Copy the null byte.
-    return ConstantInt::get(CI->getType(), FormatStr.size());
+                       DstArg, Align(1), FmtArg, Align(1),
+            ConstantInt::get(DL.getIntPtrType(CI->getContext()), N)));
+
+    if (N == FormatStr.size() + 1)
+      // Return early when the whole format string, including the final nul,
+      // has been copied.
+      return ConstantInt::get(CI->getType(), N - 1);
+
+    // When truncating the format string append a terminating nul.
+    Type *Int8Ty = B.getInt8Ty();
+    Value *NBytes = B.getInt32(N);
+    Value *DstEnd = B.CreateInBoundsGEP(Int8Ty, DstArg, NBytes, "endptr");
+    B.CreateStore(ConstantInt::get(Int8Ty, 0), DstEnd);
+    return NBytes;
   }
 
   // The remaining optimizations require the format string to be "%s" or "%c"
   // and have an extra operand.
   if (FormatStr.size() == 2 && FormatStr[0] == '%' && CI->arg_size() == 4) {
 
     // Decode the second character of the format string.
     if (FormatStr[1] == 'c') {
       if (N == 0)
         return ConstantInt::get(CI->getType(), 1);
       else if (N == 1)
         return nullptr;
 
       // snprintf(dst, size, "%c", chr) --> *(i8*)dst = chr; *((i8*)dst+1) = 0
       if (!CI->getArgOperand(3)->getType()->isIntegerTy())
         return nullptr;
       Value *V = B.CreateTrunc(CI->getArgOperand(3), B.getInt8Ty(), "char");
-      Value *Ptr = castToCStr(CI->getArgOperand(0), B);
+      Value *Ptr = castToCStr(DstArg, B);
       B.CreateStore(V, Ptr);
       Ptr = B.CreateInBoundsGEP(B.getInt8Ty(), Ptr, B.getInt32(1), "nul");
       B.CreateStore(B.getInt8(0), Ptr);
 
       return ConstantInt::get(CI->getType(), 1);
     }
 
     if (FormatStr[1] == 's') {
       // snprintf(dest, size, "%s", str) to llvm.memcpy(dest, str, len+1, 1)
       StringRef Str;
       if (!getConstantStringInfo(CI->getArgOperand(3), Str))
         return nullptr;
 
       if (N == 0)
         return ConstantInt::get(CI->getType(), Str.size());
       else if (N < Str.size() + 1)
         return nullptr;
 
       copyFlags(
-          *CI, B.CreateMemCpy(CI->getArgOperand(0), Align(1),
+          *CI, B.CreateMemCpy(DstArg, Align(1),
                               CI->getArgOperand(3), Align(1),
                               ConstantInt::get(CI->getType(), Str.size() + 1)));
 
       // The snprintf result is the unincremented number of bytes in the string.
       return ConstantInt::get(CI->getType(), Str.size());
     }
   }
   return nullptr;

llvm/test/Transforms/InstCombine/snprintf-2.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; Verify that snprintf calls with a constant size and constant string
+; with no formatting directives are transformed into memcpy.
+;
+; RUN: opt < %s -passes=instcombine -S -data-layout="E" | FileCheck %s -check-prefixes=ANY,BE
+; RUN: opt < %s -passes=instcombine -S -data-layout="e" | FileCheck %s -check-prefixes=ANY,LE
+
+@s = constant [4 x i8] c"123\00"
+
+@adst = external global [0 x i8*]
+@asiz = external global [0 x i32]
+
+declare i32 @snprintf(i8*, i64, i8*, ...)
+
+
+; Verify that all snprintf calls with a bound between INT_MAX and down
+; to 0 are transformed to memcpy.
+
+define void @fold_snprintf_cst() {
+; BE-LABEL: @fold_snprintf_cst(
+; BE-NEXT:    [[PDIMAX1:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 2147483647) to i32**), align 8
+; BE-NEXT:    store i32 825373440, i32* [[PDIMAX1]], align 1
+; BE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 0), align 4
+; BE-NEXT:    [[PD52:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 5) to i32**), align 8
+; BE-NEXT:    store i32 825373440, i32* [[PD52]], align 1
+; BE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 5), align 4
+; BE-NEXT:    [[PD43:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 4) to i32**), align 8
+; BE-NEXT:    store i32 825373440, i32* [[PD43]], align 1
+; BE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 4), align 4
+; BE-NEXT:    [[PD34:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 3) to i32**), align 8
+; BE-NEXT:    store i32 825373440, i32* [[PD34]], align 1
+; BE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 3), align 4
+; BE-NEXT:    [[PD2:%.*]] = load i8*, i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 2), align 8
+; BE-NEXT:    [[TMP1:%.*]] = bitcast i8* [[PD2]] to i16*
+; BE-NEXT:    store i16 12594, i16* [[TMP1]], align 1
+; BE-NEXT:    [[ENDPTR:%.*]] = getelementptr inbounds i8, i8* [[PD2]], i64 2
+; BE-NEXT:    store i8 0, i8* [[ENDPTR]], align 1
+; BE-NEXT:    store i32 2, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 2), align 4
+; BE-NEXT:    [[PD1:%.*]] = load i8*, i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 1), align 8
+; BE-NEXT:    store i8 49, i8* [[PD1]], align 1
+; BE-NEXT:    [[ENDPTR5:%.*]] = getelementptr inbounds i8, i8* [[PD1]], i64 1
+; BE-NEXT:    store i8 0, i8* [[ENDPTR5]], align 1
+; BE-NEXT:    store i32 1, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 1), align 4
+; BE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 0), align 4
+; BE-NEXT:    ret void
+;
+; LE-LABEL: @fold_snprintf_cst(
+; LE-NEXT:    [[PDIMAX1:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 2147483647) to i32**), align 8
+; LE-NEXT:    store i32 3355185, i32* [[PDIMAX1]], align 1
+; LE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 0), align 4
+; LE-NEXT:    [[PD52:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 5) to i32**), align 8
+; LE-NEXT:    store i32 3355185, i32* [[PD52]], align 1
+; LE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 5), align 4
+; LE-NEXT:    [[PD43:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 4) to i32**), align 8
+; LE-NEXT:    store i32 3355185, i32* [[PD43]], align 1
+; LE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 4), align 4
+; LE-NEXT:    [[PD34:%.*]] = load i32*, i32** bitcast (i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 3) to i32**), align 8
+; LE-NEXT:    store i32 3355185, i32* [[PD34]], align 1
+; LE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 3), align 4
+; LE-NEXT:    [[PD2:%.*]] = load i8*, i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 2), align 8
+; LE-NEXT:    [[TMP1:%.*]] = bitcast i8* [[PD2]] to i16*
+; LE-NEXT:    store i16 12849, i16* [[TMP1]], align 1
+; LE-NEXT:    [[ENDPTR:%.*]] = getelementptr inbounds i8, i8* [[PD2]], i64 2
+; LE-NEXT:    store i8 0, i8* [[ENDPTR]], align 1
+; LE-NEXT:    store i32 2, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 2), align 4
+; LE-NEXT:    [[PD1:%.*]] = load i8*, i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 1), align 8
+; LE-NEXT:    store i8 49, i8* [[PD1]], align 1
+; LE-NEXT:    [[ENDPTR5:%.*]] = getelementptr inbounds i8, i8* [[PD1]], i64 1
+; LE-NEXT:    store i8 0, i8* [[ENDPTR5]], align 1
+; LE-NEXT:    store i32 1, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 1), align 4
+; LE-NEXT:    store i32 3, i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 0), align 4
+; LE-NEXT:    ret void
+;
+  %fmt = getelementptr [4 x i8], [4 x i8]* @s, i32 0, i32 0
+
+  %pdimax = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 2147483647)
+  %nimax = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pdimax, i64 2147483647, i8* %fmt)
+  store i32 %nimax, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 0)
+
+  %pd5 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 5)
+  %n5 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pd5, i64 5, i8* %fmt)
+  store i32 %n5, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 5)
+
+  %pd4 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 4)
+  %n4 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pd4, i64 4, i8* %fmt)
+  store i32 %n4, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 4)
+
+  %pd3 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 3)
+  %n3 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pd3, i64 3, i8* %fmt)
+  store i32 %n3, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 3)
+
+  %pd2 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 2)
+  %n2 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pd2, i64 2, i8* %fmt)
+  store i32 %n2, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 2)
+
+  %pd1 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 1)
+  %n1 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pd1, i64 1, i8* %fmt)
+  store i32 %n1, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 1)
+
+  %pd0 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 0)
+  %n0 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pd0, i64 0, i8* %fmt)
+  store i32 %n0, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 0)
+
+  ret void
+}
+
+
+; Verify that snprintf calls with a bound greater than INT_MAX are not
+; transformed.  POSIX requires implementations to set errno to EOVERFLOW
+; so such calls could be folded to just that followed by returning -1.
+
+define void @call_snprintf_ximax_cst() {
+; ANY-LABEL: @call_snprintf_ximax_cst(
+; ANY-NEXT:    [[PDM1:%.*]] = load i8*, i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 1), align 8
+; ANY-NEXT:    [[NM1:%.*]] = call i32 (i8*, i64, i8*, ...) @snprintf(i8* noundef nonnull dereferenceable(1) [[PDM1]], i64 -1, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @s, i64 0, i64 0))
+; ANY-NEXT:    store i32 [[NM1]], i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 1), align 4
+; ANY-NEXT:    [[PDIMAXP1:%.*]] = load i8*, i8** getelementptr inbounds ([0 x i8*], [0 x i8*]* @adst, i64 0, i64 0), align 8
+; ANY-NEXT:    [[NIMAXP1:%.*]] = call i32 (i8*, i64, i8*, ...) @snprintf(i8* noundef nonnull dereferenceable(1) [[PDIMAXP1]], i64 2147483648, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @s, i64 0, i64 0))
+; ANY-NEXT:    store i32 [[NIMAXP1]], i32* getelementptr inbounds ([0 x i32], [0 x i32]* @asiz, i64 0, i64 0), align 4
+; ANY-NEXT:    ret void
+;
+  %fmt = getelementptr [4 x i8], [4 x i8]* @s, i32 0, i32 0
+
+  %pdm1 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 1)
+  %nm1 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pdm1, i64 -1, i8* %fmt)
+  store i32 %nm1, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 1)
+
+  %pdimaxp1 = load i8*, i8** getelementptr ([0 x i8*], [0 x i8*]* @adst, i32 0, i32 0)
+  %nimaxp1 = call i32 (i8*, i64, i8*, ...) @snprintf(i8* %pdimaxp1, i64 2147483648, i8* %fmt)
+  store i32 %nimaxp1, i32* getelementptr ([0 x i32], [0 x i32]* @asiz, i32 0, i32 0)
+
+  ret void
+}