[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Dimitry Andric via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3db6783d8a7d: Check result of emitStrLen before passing it 
to CreateGEP (authored by dim).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143

Files:
  llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
  llvm/test/Transforms/InstCombine/pr43081.ll


Index: llvm/test/Transforms/InstCombine/pr43081.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/pr43081.ll
@@ -0,0 +1,15 @@
+; 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
+}
Index: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -364,8 +364,8 @@
   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;
   }
 


Index: llvm/test/Transforms/InstCombine/pr43081.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/pr43081.ll
@@ -0,0 +1,15 @@
+; 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
+}
Index: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -364,8 +364,8 @@
   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;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

thx.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Dimitry Andric via Phabricator via cfe-commits]

dim updated this revision to Diff 229172.
dim added a comment.

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

Also use code style of nearest preceding constructs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143

Files:
  llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
  llvm/test/Transforms/InstCombine/pr43081.ll


Index: llvm/test/Transforms/InstCombine/pr43081.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/pr43081.ll
@@ -0,0 +1,15 @@
+; 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
+}
Index: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -364,8 +364,8 @@
   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;
   }
 


Index: llvm/test/Transforms/InstCombine/pr43081.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/pr43081.ll
@@ -0,0 +1,15 @@
+; 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
+}
Index: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -364,8 +364,8 @@
   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;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Dimitry Andric via Phabricator via cfe-commits]

dim added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:370
+: nullptr;
+}
 return nullptr;

dim wrote:
> jdoerfert wrote:
> > Consistent style please:
> > 
> > ```
> > if (Value *StrLen = emitStrLen(SrcStr, B, DL, TLI)
> >   return B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr");
> > ```
> 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...
Consistent with the two prior lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Dimitry Andric via Phabricator via cfe-commits]

dim marked an inline comment as done.
dim added inline comments.



Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:370
+: nullptr;
+}
 return nullptr;

jdoerfert wrote:
> Consistent style please:
> 
> ```
> if (Value *StrLen = emitStrLen(SrcStr, B, DL, TLI)
>   return B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr");
> ```
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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

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.




Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:370
+: nullptr;
+}
 return nullptr;

Consistent style please:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Dimitry Andric via Phabricator via cfe-commits]

dim added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70143/new/

https://reviews.llvm.org/D70143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

[Dimitry Andric via Phabricator via cfe-commits]

dim created this revision.
dim added reviewers: xbolva00, spatel, jdoerfert, efriedma.
Herald added subscribers: cfe-commits, hiraditya.
Herald added projects: clang, LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70143

Files:
  clang/test/CodeGen/builtin-replace-strchr-with-strlen.c
  llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp


Index: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -363,9 +363,11 @@
   // 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 (CharC->isZero()) { // strchr(p, 0) -> p + strlen(p)
+  Value *StrLen = emitStrLen(SrcStr, B, DL, TLI);
+  return StrLen ? B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr")
+: nullptr;
+}
 return nullptr;
   }
 
Index: clang/test/CodeGen/builtin-replace-strchr-with-strlen.c
===
--- /dev/null
+++ clang/test/CodeGen/builtin-replace-strchr-with-strlen.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple x86_64-- -S -O1 -fno-builtin-strlen %s -o - | 
FileCheck %s
+char *strchr(const char *, int);
+char *b(char *a) {
+  return strchr(a, '\0');
+// CHECK: jmp  strchr
+}


Index: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -363,9 +363,11 @@
   // 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 (CharC->isZero()) { // strchr(p, 0) -> p + strlen(p)
+  Value *StrLen = emitStrLen(SrcStr, B, DL, TLI);
+  return StrLen ? B.CreateGEP(B.getInt8Ty(), SrcStr, StrLen, "strchr")
+: nullptr;
+}
 return nullptr;
   }
 
Index: clang/test/CodeGen/builtin-replace-strchr-with-strlen.c
===
--- /dev/null
+++ clang/test/CodeGen/builtin-replace-strchr-with-strlen.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple x86_64-- -S -O1 -fno-builtin-strlen %s -o - | FileCheck %s
+char *strchr(const char *, int);
+char *b(char *a) {
+  return strchr(a, '\0');
+// CHECK: jmp	strchr
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits