[Analysis] Validate the return type of s(n)printf libcalls

[mstorsjo]

Summary

If the sprintf function is static, earlier optimization passes could optimize out the return value altogether, and make it void, which could break optimizations of this libcall that touch the return value.

[spatel]

I haven't looked in detail at the failure or discussion in D46285 or https://bugs.llvm.org/show_bug.cgi?id=37408 , so I might be missing something.
Why are we getting this far in the 1st place? Shouldn't we have avoided this problem in TargetLibraryInfoImpl::isValidProtoForLibFunc() by checking that the return type is wrong?

[mstorsjo]

In D46752#1095878, @spatel wrote:

I haven't looked in detail at the failure or discussion in D46285 or https://bugs.llvm.org/show_bug.cgi?id=37408 , so I might be missing something.
Why are we getting this far in the 1st place? Shouldn't we have avoided this problem in TargetLibraryInfoImpl::isValidProtoForLibFunc() by checking that the return type is wrong?

Indeed, that's also doable - I'm not familiar with this layer at all so I didn't know about that check. The existing checks for sprintf/snprintf don't check the return type, but by adding a return type check there, it also works fine.

[spatel]

lib/Analysis/TargetLibraryInfo.cpp

696 ↗

(On Diff #146320)

Is it possible to make this check stronger:
FTy.getReturnType()->isIntegerTy(32) ?

test/Transforms/InstCombine/sprintf-void.ll

13 ↗

(On Diff #146320)

I always make this suggestion for instcombine tests: it's better to use the script at utils/update_test_checks.py to auto-generate the CHECK lines than to manually write them.

[mstorsjo]

lib/Analysis/TargetLibraryInfo.cpp

696 ↗

(On Diff #146320)

I guess that'd be ok as well, although it would disqualify any ILP64 platform. But I guess that's just a theoretical concern, given the number of ILP64 platforms.

test/Transforms/InstCombine/sprintf-void.ll

13 ↗

(On Diff #146320)

Ok, will do.

[mstorsjo]

Made the requested adjustments.

[mstorsjo]

lib/Analysis/TargetLibraryInfo.cpp

696 ↗

(On Diff #146320)

I see that other existing checks for things that should be int also uses isIntegerTy(32), so I changed it as requested.

[spatel]

LGTM - thanks!
Note (future patches): just glancing at the surrounding cases, it seems like we're missing similar return type checks for other calls.

[mstorsjo]

In D46752#1096016, @spatel wrote:

LGTM - thanks!
Note (future patches): just glancing at the surrounding cases, it seems like we're missing similar return type checks for other calls.

Yup. Although I guess it's mostly a question of what optimizations require what details to match; for this particular optimization, it could have been made to handle the case of a void return as well (but it's not necessarily worth to).

[xbolva00]

After you merge this, revert also https://reviews.llvm.org/rL332043

[xbolva00]

I commited it again, you do not need to revert your revert.

[efriedma]

I'm not convinced this really solves the problem correctly... I mean, obviously we shouldn't crash, but this doesn't deal with the underlying issue: once we transform the internal function, it is no longer the C library function, and that difference may not be reflected in the signature. See http://lists.llvm.org/pipermail/llvm-dev/2017-October/118578.html .

[mstorsjo]

In D46752#1096183, @xbolva00 wrote:

I commited it again, you do not need to revert your revert.

Thanks - the codebase that triggered the error seems to build fine now still with this reapplied.

In D46752#1096337, @efriedma wrote:

I'm not convinced this really solves the problem correctly... I mean, obviously we shouldn't crash, but this doesn't deal with the underlying issue: once we transform the internal function, it is no longer the C library function, and that difference may not be reflected in the signature. See http://lists.llvm.org/pipermail/llvm-dev/2017-October/118578.html .

Hm, indeed, there's clearly more aspects to it. I take it there was no further progress since, on the ideas presented there?

[efriedma]

No, no progress on the question of the semantics of library functions with internal linkage. (It usually isn't a problem in practice because the functions in question are built with -fno-builtin, so nobody has really spent any time.)

llvm/trunk/lib/Analysis/TargetLibraryInfo.cpp

case LibFunc_strndup:
     return (NumParams >= 1 && FTy.getReturnType()->isPointerTy() &&
             FTy.getParamType(0)->isPointerTy());
   case LibFunc_sscanf:
   case LibFunc_stat:
   case LibFunc_statvfs:
   case LibFunc_siprintf:
   case LibFunc_sprintf:
     return (NumParams >= 2 && FTy.getParamType(0)->isPointerTy() &&
-            FTy.getParamType(1)->isPointerTy());
+            FTy.getParamType(1)->isPointerTy() &&
+            FTy.getReturnType()->isIntegerTy(32));
   case LibFunc_snprintf:
     return (NumParams == 3 && FTy.getParamType(0)->isPointerTy() &&
-            FTy.getParamType(2)->isPointerTy());
+            FTy.getParamType(2)->isPointerTy() &&
+            FTy.getReturnType()->isIntegerTy(32));
   case LibFunc_setitimer:
     return (NumParams == 3 && FTy.getParamType(1)->isPointerTy() &&
             FTy.getParamType(2)->isPointerTy());
   case LibFunc_system:
     return (NumParams == 1 && FTy.getParamType(0)->isPointerTy());
   case LibFunc_malloc:
     return (NumParams == 1 && FTy.getReturnType()->isPointerTy());
   case LibFunc_memcmp:

llvm/trunk/test/Transforms/InstCombine/sprintf-void.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128"
+
+@hello_world = constant [13 x i8] c"hello world\0A\00"
+
+declare void @sprintf(i8*, i8*, ...)
+
+; Check that a sprintf call, that would otherwise be optimized, but with
+; optimized out return type, doesn't crash the optimizer.
+
+define void @test_simplify1(i8* %dst) {
+; CHECK-LABEL: @test_simplify1(
+; CHECK-NEXT:    call void (i8*, i8*, ...) @sprintf(i8* [[DST:%.*]], i8* getelementptr inbounds ([13 x i8], [13 x i8]* @hello_world, i32 0, i32 0))
+; CHECK-NEXT:    ret void
+;
+  %fmt = getelementptr [13 x i8], [13 x i8]* @hello_world, i32 0, i32 0
+  call void (i8*, i8*, ...) @sprintf(i8* %dst, i8* %fmt)
+  ret void
+}