This is an archive of the discontinued LLVM Phabricator instance.

Check result of emitStrLen before passing it to CreateGEP
ClosedPublic

Authored by dim on Nov 12 2019, 1:33 PM.

Details

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.

Event Timeline

dim created this revision.Nov 12 2019, 1:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 12 2019, 1:33 PM

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

dim added a comment.Nov 12 2019, 1:56 PM

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.

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.

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 marked an inline comment as done.Nov 13 2019, 6:52 AM
dim added inline comments.
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 added inline comments.Nov 13 2019, 8:13 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
371

Consistent with the two prior lines.

dim added a comment.Nov 13 2019, 11:17 AM

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

dim updated this revision to Diff 229172.Nov 13 2019, 1:19 PM

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

Also use code style of nearest preceding constructs.

This revision is now accepted and ready to land.Nov 13 2019, 2:46 PM
This revision was automatically updated to reflect the committed changes.