Revert "[SLC] Preserve attrs for strncpy(x, "", y) -> memset(align 1 x, '\0', y)"

[krasimir]

Summary

This reverts commit r372101.

Causes ASAN build bot failures:

http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/14176
From http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/14176/steps/64-bit%20check-asan/logs/stdio:

[ RUN      ] AddressSanitizer.StrNCatOOBTest
/home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/compiler-rt/lib/asan/tests/asan_str_test.cpp:462: Failure
Death test: strncat(to - 1, from, 0)
    Result: failed to die.

[bkramer]

lg

[lebedev.ri]

There was several commits touching this code in rapid succession.
This seems to revert rL372101, but what about rL372097, rL372096, and possibly relevant part of D53342/rL372091 ?

[xbolva00]

Ok, sorry. Will investigate.

[xbolva00]

In D67658#1672604, @lebedev.ri wrote:

There was several commits touching this code in rapid succession.
This seems to revert rL372101, but what about rL372097, rL372096, and possibly relevant part of D53342/rL372091 ?

Yeah, but unfortunely. My system GCC 7 does not warn me about that "attrs" bug :/ (testing compiler.h fix currently).

The failed test mentions strncat so yeah, maybe this revert is incorrect - but let's just do it to see.

I checked the new code for strncat again and I see nothing bad.

annotateNonNullBasedOnAccess(CI, 0);
 if (isKnownNonZero(Size, DL))
   annotateNonNullBasedOnAccess(CI, 1);

 // We don't do anything if length is not constant.
 ConstantInt *LengthArg = dyn_cast<ConstantInt>(Size);
 if (LengthArg) {
   Len = LengthArg->getZExtValue();
   // strncat(x, c, 0) -> x
   if (!Len)
     return Dst;
 } else {
   return nullptr;
 }

so for "strncat(to - 1, from, 0)" we eliminate strncat call and return "to - 1" ptr. (patch does not change the behaviour here).

[xbolva00]

so for "strncat(to - 1, from, 0)" we eliminate strncat call and return "to - 1" ptr. (patch does not change the behaviour here).

No.

char *foo(char *s, char *p) {

return strncat(s, p, 0);

}

GCC
foo(char*, char*):

movq    %rdi, %rax
ret

Clang before https://reviews.llvm.org/rL372091
_Z3fooPcS_: # @_Z3fooPcS_
.cfi_startproc

%bb.0:

xorl %edx, %edx
jmp strncat # TAILCALL

Clang after
_Z3fooPcS_:
.LFB27:
.cfi_startproc
movq %rdi, %rax
ret

But Clang already had pattern for that!

// Handle the simple, do-nothing cases:
// strncat(x, "", c) -> x
// strncat(x,  c, 0) -> x
if (SrcLen == 0 || Len == 0)
  return Dst;

But somehow it wasn't working. I had to reorder the code a bit and it works now (we hadn't test for it, rL372091 added it).

@krasimir for "strncat(s, p, 0);" there is no libcall and probably this is reason why test failed. Could you please fix test for new behaviour? (well, either remove it or change to 1), please? And reland original commit which was reverted.

I am sorry for late investigation, I was out of PC..

Edit: I landed the fix, watching the bot now..

[krasimir]

Hey! Thank you very much for your investigations!

Sorry for the late reply, I was out. I was just trying to fix several build bot failures to unblock continuous builds. Wasn't familiar with the details of the patches.

I see you rolled all the fixes for this and rolled forward your updates in r372141 - r372143.

It's pretty cool that we don't get a call in this case anymore!

llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp

if (SrcLen) {
     annotateDereferenceableBytes(CI, 1, SrcLen);
     --SrcLen; // Unbias length.
   } else {
     return nullptr;
   }
 
   if (SrcLen == 0) {
     // strncpy(x, "", y) -> memset(align 1 x, '\0', y)
-    CallInst *NewCI = B.CreateMemSet(Dst, B.getInt8('\0'), Size, 1);
-    AttrBuilder ArgAttrs(CI->getAttributes().getParamAttributes(0));
-    NewCI->setAttributes(NewCI->getAttributes().addParamAttributes(
-        CI->getContext(), 0, ArgAttrs));
+    B.CreateMemSet(Dst, B.getInt8('\0'), Size, 1);
     return Dst;
   }
 
   // Let strncpy handle the zero padding
   if (Len > SrcLen + 1)
     return nullptr;
 
   Type *PT = Callee->getFunctionType()->getParamType(0);

llvm/trunk/test/Transforms/InstCombine/strncpy-1.ll

 ; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* nonnull align 1 dereferenceable(5) [[DST:%.*]], i8* nonnull align 1 dereferenceable(6) getelementptr inbounds ([6 x i8], [6 x i8]* @hello, i32 0, i32 0), i32 5, i1 false)
 ; CHECK-NEXT:    ret i8* [[DST]]
 ;
   %src = getelementptr [6 x i8], [6 x i8]* @hello, i32 0, i32 0
   %ret = call i8* @strncpy(i8* nonnull %dst, i8* nonnull %src, i32 5)
   ret i8* %ret
 }
 
-define i8* @test3(i8* %dst, i32 %n) {
-; CHECK-LABEL: @test3(
-; CHECK-NEXT:    call void @llvm.memset.p0i8.i32(i8* noalias nonnull align 1 dereferenceable(5) [[DST:%.*]], i8 0, i32 5, i1 false)
-; CHECK-NEXT:    ret i8* [[DST]]
-;
-  %src = getelementptr [1 x i8], [1 x i8]* @null, i32 0, i32 0
-  %ret = call i8* @strncpy(i8* noalias nonnull %dst, i8* nonnull %src, i32 5);
-  ret i8* %ret
-}
-
 ; Check cases that shouldn't be simplified.
 
 define void @test_no_simplify1() {
 ; CHECK-LABEL: @test_no_simplify1(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call i8* @strncpy(i8* nonnull dereferenceable(1) getelementptr inbounds ([32 x i8], [32 x i8]* @a, i32 0, i32 0), i8* nonnull dereferenceable(1) getelementptr inbounds ([32 x i8], [32 x i8]* @b, i32 0, i32 0), i32 32)
 ; CHECK-NEXT:    ret void
 ;
   %dst = getelementptr [32 x i8], [32 x i8]* @a, i32 0, i32 0