Check result of emitStrLen before passing it to CreateGEP

[dim]

Summary

This fixes PR43081, where the transformation of `strchr(p, 0) -> p +
strlen(p) can cause a segfault, if -fno-builtin-strlen` is used. In
that case, emitStrLen returns nullptr, which CreateGEP is not designed
to handle. Also add the minimized code from the PR as a test case.

[lebedev.ri]

This should have a llvm ir test in llvm/test/transforms/instcombine i think, not a clang test.

[dim]

In D70143#1742885, @lebedev.ri wrote:

This should have a llvm ir test in llvm/test/transforms/instcombine i think, not a clang test.

Yes, I thought so too, but I could not get it to work (i.e. crash) with LLVM IR. I just don't understand how opt works, and it does not have a -fno-builtin-strlen option either. Therefore, I made it work with clang, as having a working test is better than no test at all. But I'm fine with leaving out the test, it was just for completeness' sake.

[efriedma]

It wouldn't be that hard to add an equivalent flag to opt; just a matter of calling the API on TargetLibraryInfo. -disable-simplify-libcalls already exists, but I guess that's not quite what you want.

[jdoerfert]

One style comment, patch looks conceptually fine to me otherwise. I'll wait to accept on how we fall on the test issue: I'd opt for an opt test if possible.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

371

Consistent style please:

if (Value *StrLen = emitStrLen(SrcStr, B, DL, TLI)
  return B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr");

[dim]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

371

Consistent with what? :) In this same file, I see at least the following calls to emitStrLen, some of which use the if(!x) return nullptr spelling, others which use return x ? y : nullptr:

Value *DstLen = emitStrLen(Dst, B, DL, TLI);
if (!DstLen)
  return nullptr;

if (Dst == Src) { // stpcpy(x,x)  -> x+strlen(x)
  Value *StrLen = emitStrLen(Src, B, DL, TLI);
  return StrLen ? B.CreateInBoundsGEP(B.getInt8Ty(), Dst, StrLen) : nullptr;
}

Value *StrLen = emitStrLen(CI->getArgOperand(1), B, DL, TLI);
if (!StrLen)
  return nullptr;

Value *Len = emitStrLen(CI->getArgOperand(2), B, DL, TLI);
if (!Len)
  return nullptr;

Value *StrLen = emitStrLen(Src, B, DL, TLI);
return StrLen ? B.CreateInBoundsGEP(B.getInt8Ty(), Dst, StrLen) : nullptr;

But I'm fine with whatever you are suggesting, obviously. It just seems strange to introduce yet another spelling variant, making it less consistent, not more...

[jdoerfert]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

371

Consistent with the two prior lines.

[dim]

I submitted D70193 for adding a -disable-builtin option to opt. Once that is committed, this review can continue.

[dim]

Now opt supports -disable-builtin, move the test to llvm/test/Transforms/InstCombine.

Also use code style of nearest preceding constructs.

[efriedma]

LGTM

[jdoerfert]

thx.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

return emitMemChr(SrcStr, CI->getArgOperand(1), // include nul.
                       B, DL, TLI);
   }
 
   // Otherwise, the character is a constant, see if the first argument is
   // a string literal.  If so, we can constant fold.
   StringRef Str;
   if (!getConstantStringInfo(SrcStr, Str)) {
     if (CharC->isZero()) // strchr(p, 0) -> p + strlen(p)
-      return B.CreateGEP(B.getInt8Ty(), SrcStr, emitStrLen(SrcStr, B, DL, TLI),
-                         "strchr");
+      if (Value *StrLen = emitStrLen(SrcStr, B, DL, TLI))
+        return B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr");
     return nullptr;
   }
 
   // Compute the offset, make sure to handle the case when we're searching for
   // zero (a weird way to spell strlen).
   size_t I = (0xFF & CharC->getSExtValue()) == 0
                  ? Str.size()
                  : Str.find(CharC->getSExtValue());
   if (I == StringRef::npos) // Didn't find the char.  strchr returns null.
     return Constant::getNullValue(CI->getType());
 

llvm/test/Transforms/InstCombine/pr43081.ll

+; RUN: opt < %s -instcombine -disable-builtin strlen -S | FileCheck %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+
+declare i8* @strchr(i8*, i32)
+
+define i8* @pr43081(i8* %a) {
+entry:
+  %a.addr = alloca i8*, align 8
+  store i8* %a, i8** %a.addr, align 8
+  %0 = load i8*, i8** %a.addr, align 8
+  %call = call i8* @strchr(i8* %0, i32 0)
+  ret i8* %call
+; CHECK: call i8* @strchr
+}