Improve support of GNU mempcpy

[serge-sans-paille]

Summary

• Lower to the memcpy intrinsic
• Raise warnings when size/bounds are known

[serge-sans-paille]

On going validation: https://github.com/serge-sans-paille/llvm-project/pull/5/checks

[serge-sans-paille]

In D71374#1780402, @serge-sans-paille wrote:

On going validation: https://github.com/serge-sans-paille/llvm-project/pull/5/checks

Validation succeeded, waiting for review!

[Jim]

clang/lib/CodeGen/CGBuiltin.cpp

2518

Is it an error here?

It should be:

if (BuiltinID == Builtin::BImempcpy ||
    BuiltinID == Builtin::BI__builtin_mempcpy)
    return RValue::get(Builder.CreateGEP(Dest.getPointer(), SizeVal));
else
    return RValue::get(Dest.getPointer());

[serge-sans-paille]

@Jim obviously :-) Thanks for spotting that.

[Jim]

I am curious what is difference of code generation after applying your changes?

[xbolva00]

Please add a IR codegen test.

[serge-sans-paille]

In D71374#1783032, @Jim wrote:

I am curious what is difference of code generation after applying your changes?

[[updated]]
Before, when compiling

#define _GNU_SOURCE
#include <string.h>

void* foo(void* to, void* from, unsigned n) {
  return mempcpy(mempcpy(to, from, n), from, n);
}

We get (clang -O3)

define dso_local i8* @foo(i8* %0, i8* nocapture readonly %1, i32 %2) local_unnamed_addr #0 {
  %4 = zext i32 %2 to i64
  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 %1, i64 %4, i1 false) #2
  %5 = getelementptr inbounds i8, i8* %0, i64 %4
  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %5, i8* align 1 %1, i64 %4, i1 false) #2
  %6 = getelementptr inbounds i8, i8* %5, i64 %4
  ret i8* %6
}

And we now get

define dso_local i8* @foo(i8* %to, i8* nocapture readonly %from, i32 %n) local_unnamed_addr #0 {
entry:
  %conv = zext i32 %n to i64
  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %to, i8* align 1 %from, i64 %conv, i1 false)
  %0 = getelementptr i8, i8* %to, i64 %conv
  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 %from, i64 %conv, i1 false)
  %1 = getelementptr i8, i8* %0, i64 %conv
  ret i8* %1
}

So no real change apart from the inbound keyword that I forgot to add.

[xbolva00]

LLVM already converts mempcpy to memcpy..

[serge-sans-paille]

In D71374#1783245, @xbolva00 wrote:

LLVM already converts mempcpy to memcpy..

Indeed, the clang version I was using as base reference was clang-9, and the mempcpy optimisation at IR level got introduced after that.
Nevertheless, this patch does much more than lowering mempcpy, it also triggers new warnings, so It's still useful that clang understands it.

@xbolva00 should I remove the lowering part and leave it to llvm?

[serge-sans-paille]

[[edited the example to reflect actual state]]

[xbolva00]

@xbolva00 should I remove the lowering part and leave it to llvm?

Probably okay to leave it + inbounds, but please add a test.

[serge-sans-paille]

Added test cases + inbound GEP

[serge-sans-paille]

@xbolva00 : validation is ok: https://github.com/serge-sans-paille/llvm-project/pull/5/checks

[serge-sans-paille]

@xbolva00 : gentle reminder :-)

[xbolva00]

LG

clang/include/clang/Basic/Builtins.def

 
 // Non-C library functions, active in GNU mode only.
 // Functions with (returns_twice) attribute (marked as "j") are still active in
 // all languages, because losing this attribute would result in miscompilation
 // when these functions are used in non-GNU mode. PR16138.
 LIBBUILTIN(alloca, "v*z",         "f",     "stdlib.h", ALL_GNU_LANGUAGES)
 // POSIX string.h
 LIBBUILTIN(memccpy, "v*v*vC*iz",  "f",     "string.h", ALL_GNU_LANGUAGES)
+LIBBUILTIN(mempcpy, "v*v*vC*z",   "f",     "string.h", ALL_GNU_LANGUAGES)
 LIBBUILTIN(stpcpy, "c*c*cC*",     "f",     "string.h", ALL_GNU_LANGUAGES)
 LIBBUILTIN(stpncpy, "c*c*cC*z",   "f",     "string.h", ALL_GNU_LANGUAGES)
 LIBBUILTIN(strdup, "c*cC*",       "f",     "string.h", ALL_GNU_LANGUAGES)
 LIBBUILTIN(strndup, "c*cC*z",     "f",     "string.h", ALL_GNU_LANGUAGES)
 // POSIX strings.h
 LIBBUILTIN(index, "c*cC*i",       "f",     "strings.h", ALL_GNU_LANGUAGES)
 LIBBUILTIN(rindex, "c*cC*i",      "f",     "strings.h", ALL_GNU_LANGUAGES)
 LIBBUILTIN(bzero, "vv*z",         "f",     "strings.h", ALL_GNU_LANGUAGES)

clang/lib/AST/Decl.cpp

unsigned FunctionDecl::getMemoryFunctionKind() const {
   case Builtin::BImemset:
     return Builtin::BImemset;
 
   case Builtin::BI__builtin_memcpy:
   case Builtin::BI__builtin___memcpy_chk:
   case Builtin::BImemcpy:
     return Builtin::BImemcpy;
 
+  case Builtin::BI__builtin_mempcpy:
+  case Builtin::BI__builtin___mempcpy_chk:
+  case Builtin::BImempcpy:
+    return Builtin::BImempcpy;
+
   case Builtin::BI__builtin_memmove:
   case Builtin::BI__builtin___memmove_chk:
   case Builtin::BImemmove:
     return Builtin::BImemmove;
 
   case Builtin::BIstrlcpy:
   case Builtin::BI__builtin___strlcpy_chk:
     return Builtin::BIstrlcpy;
case Builtin::BIbzero:
     return Builtin::BIbzero;
 
   default:
     if (isExternC()) {
       if (FnInfo->isStr("memset"))
         return Builtin::BImemset;
       else if (FnInfo->isStr("memcpy"))
         return Builtin::BImemcpy;
+      else if (FnInfo->isStr("mempcpy"))
+        return Builtin::BImempcpy;
       else if (FnInfo->isStr("memmove"))
         return Builtin::BImemmove;
       else if (FnInfo->isStr("memcmp"))
         return Builtin::BImemcmp;
       else if (FnInfo->isStr("bcmp"))
         return Builtin::BIbcmp;
       else if (FnInfo->isStr("strncpy"))
         return Builtin::BIstrncpy;

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_bzero: {
     Address Dest = EmitPointerWithAlignment(E->getArg(0));
     Value *SizeVal = EmitScalarExpr(E->getArg(1));
     EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),
                         E->getArg(0)->getExprLoc(), FD, 0);
     Builder.CreateMemSet(Dest, Builder.getInt8(0), SizeVal, false);
     return RValue::get(nullptr);
   }
   case Builtin::BImemcpy:
-  case Builtin::BI__builtin_memcpy: {
+  case Builtin::BI__builtin_memcpy:
+  case Builtin::BImempcpy:
+  case Builtin::BI__builtin_mempcpy: {
     Address Dest = EmitPointerWithAlignment(E->getArg(0));
     Address Src = EmitPointerWithAlignment(E->getArg(1));
     Value *SizeVal = EmitScalarExpr(E->getArg(2));
     EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),
                         E->getArg(0)->getExprLoc(), FD, 0);
     EmitNonNullArgCheck(RValue::get(Src.getPointer()), E->getArg(1)->getType(),
                         E->getArg(1)->getExprLoc(), FD, 1);
     Builder.CreateMemCpy(Dest, Src, SizeVal, false);
+    if (BuiltinID == Builtin::BImempcpy ||
+        BuiltinID == Builtin::BI__builtin_mempcpy)
+      return RValue::get(Builder.CreateInBoundsGEP(Dest.getPointer(), SizeVal));
+    else
       return RValue::get(Dest.getPointer());
   }
 
   case Builtin::BI__builtin_char_memchr:
     BuiltinID = Builtin::BI__builtin_memchr;
     break;
 
   case Builtin::BI__builtin___memcpy_chk: {
     // fold __builtin_memcpy_chk(x, y, cst1, cst2) to memcpy iff cst1<=cst2.

clang/lib/Sema/SemaChecking.cpp

void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
   case Builtin::BI__builtin___memcpy_chk:
   case Builtin::BI__builtin___memmove_chk:
   case Builtin::BI__builtin___memset_chk:
   case Builtin::BI__builtin___strlcat_chk:
   case Builtin::BI__builtin___strlcpy_chk:
   case Builtin::BI__builtin___strncat_chk:
   case Builtin::BI__builtin___strncpy_chk:
   case Builtin::BI__builtin___stpncpy_chk:
-  case Builtin::BI__builtin___memccpy_chk: {
+  case Builtin::BI__builtin___memccpy_chk:
+  case Builtin::BI__builtin___mempcpy_chk: {
     DiagID = diag::warn_builtin_chk_overflow;
     IsChkVariant = true;
     SizeIndex = TheCall->getNumArgs() - 2;
     ObjectIndex = TheCall->getNumArgs() - 1;
     break;
   }
 
   case Builtin::BI__builtin___snprintf_chk:
case Builtin::BI__builtin_stpncpy: {
     break;
   }
 
   case Builtin::BImemcpy:
   case Builtin::BI__builtin_memcpy:
   case Builtin::BImemmove:
   case Builtin::BI__builtin_memmove:
   case Builtin::BImemset:
-  case Builtin::BI__builtin_memset: {
+  case Builtin::BI__builtin_memset:
+  case Builtin::BImempcpy:
+  case Builtin::BI__builtin_mempcpy: {
     DiagID = diag::warn_fortify_source_overflow;
     SizeIndex = TheCall->getNumArgs() - 1;
     ObjectIndex = 0;
     break;
   }
   case Builtin::BIsnprintf:
   case Builtin::BI__builtin_snprintf:
   case Builtin::BIvsnprintf:

clang/test/Analysis/bstring.c

void mempcpy1 () {
   mempcpy(dst, src, 5); // expected-warning{{Memory copy function accesses out-of-bound array element}}
 }
 
 void mempcpy2 () {
   char src[] = {1, 2, 3, 4};
   char dst[1];
 
   mempcpy(dst, src, 4); // expected-warning{{Memory copy function overflows destination buffer}}
+#ifndef VARIANT
+// expected-warning@-2{{'mempcpy' will always overflow; destination buffer has size 1, but size argument is 4}}
+#endif
 }
 
 void mempcpy3 () {
   char src[] = {1, 2, 3, 4};
   char dst[3];
 
   mempcpy(dst+1, src+2, 2); // no-warning
 }
 
 void mempcpy4 () {
   char src[] = {1, 2, 3, 4};
   char dst[10];
 
   mempcpy(dst+2, src+2, 3); // expected-warning{{Memory copy function accesses out-of-bound array element}}
 }
 
 void mempcpy5() {
   char src[] = {1, 2, 3, 4};
   char dst[3];
 
   mempcpy(dst+2, src+2, 2); // expected-warning{{Memory copy function overflows destination buffer}}
+#ifndef VARIANT
+// expected-warning@-2{{'mempcpy' will always overflow; destination buffer has size 1, but size argument is 2}}
+#endif
 }
 
 void mempcpy6() {
   int a[4] = {0};
   mempcpy(a, a, 8); // expected-warning{{overlapping}}  
 }
 
 void mempcpy7() {

clang/test/CodeGen/mempcpy-libcall.c

+// RUN: %clang_cc1 -emit-llvm < %s| FileCheck %s
+
+typedef __SIZE_TYPE__ size_t;
+
+void *mempcpy(void *, void const *, size_t);
+
+char *test(char *d, char *s, size_t n) {
+  // CHECK:      call void @llvm.memcpy.p0i8.p0i8.i64(i8* {{.*}} %[[REG1:[^ ]+]], i8* {{.*}} %1, i64 %[[REG2:[^ ]+]], i1 false)
+  // CHECK-NEXT: %[[REGr:[^ ]+]] = getelementptr inbounds i8, i8* %[[REG1]], i64 %[[REG2]]
+  // CHECK-NEXT: ret i8* %[[REGr]]
+  return mempcpy(d, s, n);
+}