[Builtins] Teach Clang about memccpy

[xbolva00]

Summary

Hopefully, -fno-builtin-memccpy will work now. Required for https://reviews.llvm.org/D67986.

[xbolva00]

include/clang/Basic/Builtins.def

903 ↗

(On Diff #222975)

Well, technically, C 20.

[aaron.ballman]

Test cases?

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

GCC doesn't seem to have __builtin_memccpy? https://godbolt.org/z/jbthQ3

[xbolva00]

It is not very obvious for me what kind of test should be done here. I would be grateful if you could show me an example in tree.

Exactly for -fno-builtin-memccpy case?

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

Ok, I will drop it.

[aaron.ballman]

In D68377#1693490, @xbolva00 wrote:

It is not very obvious for me what kind of test should be done here. I would be grateful if you could show me an example in tree.

I was thinking of something in CodeGen, like CodeGen/builtin-memfns.c

Exactly for -fno-builtin-memccpy case?

That would be another good test case.

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

If you drop it, won't that lose the builtin? I was mostly thinking it's in the wrong part of the list of builtins.

[xbolva00]

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

Rebuilding LLVM + Clang in progress so I just checked it in godbolt with "strtol" - defined only as LIBBUILTIN, no __builtin version.

nobuiltin attribute is correctly handled, so I think it will work.
https://godbolt.org/z/Olfv-w

[aaron.ballman]

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

Ah, I see the issue better now. The description for this review was very terse and it wasn't immediately clear what problem you were trying to solve.

Yeah, I think this declaration can go away.

983 ↗

(On Diff #223017)

Isn't memccpy supported by Visual Studio? What should happen there?

[xbolva00]

include/clang/Basic/Builtins.def

983 ↗

(On Diff #223017)

”This POSIX function is deprecated. Use the ISO C++ conformant _memccpy instead.“

[aaron.ballman]

include/clang/Basic/Builtins.def

983 ↗

(On Diff #223017)

Thanks for verifying it's still supported (deprecated != unsupported). We do support other Microsoft builtins, so should this one be added?

[xbolva00]

include/clang/Basic/Builtins.def

983 ↗

(On Diff #223017)

Ok, I will add it.

One last question, should I somehow represent that memccpy is in C 20 (if so, how) ?

[aaron.ballman]

include/clang/Basic/Builtins.def

983 ↗

(On Diff #223017)

You should probably start a new section for C2x library functions and add memccpy, strdup, and strndup to it.

Also, just to double-check: we don't encode restrict into builtin signatures, do we? (Should we?)

[xbolva00]

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

Yeah, I am sorry, more context why this is needed:

https://reviews.llvm.org/D67986

[xbolva00]

include/clang/Basic/Builtins.def

983 ↗

(On Diff #223017)

You should probably start a new section for C2x library functions and add memccpy, strdup, and strndup to it.

Okay, thanks - probably not needed for now (should be separate patch to handle new functions in C 2X anyway; maybe more functions to be added?)

Also, just to double-check: we don't encode restrict into builtin signatures, do we? (Should we?)

Maybe catch very simple overlap issues? I dont think this is worth it (we have sanitizers). LLVM annotates libc functions with noalias anyway.

[aaron.ballman]

include/clang/Basic/Builtins.def

483 ↗

(On Diff #223017)

Thank you for the extra context!

983 ↗

(On Diff #223017)

Okay, thanks - probably not needed for now (should be separate patch to handle new functions in C 2X anyway; maybe more functions to be added?)

Yeah, can totally be done in a separate patch.

Maybe catch very simple overlap issues? I dont think this is worth it (we have sanitizers). LLVM annotates libc functions with noalias anyway.

Sanitizers don't run everywhere, but more importantly, I was wondering about the possible optimization opportunities by noting which arguments cannot overlap with one another (perhaps noalias covers that; I'm less familiar with the llvm side of things).

However, I looked at the file and see we don't have any encodings for it, so nothing to be done here.

[xbolva00]

include/clang/Basic/Builtins.def

983 ↗

(On Diff #223017)

I was wondering about the possible optimization opportunities by noting which arguments cannot overlap with one another

This is implemented in LLVM.

For example: https://godbolt.org/z/E8x2Ev (see line 10).

Sanitizers don't run everywhere

GCC emits a lot of warnings from middle-end. While I dont like some implementations which are part of regular pass, but extra warning pass just to check specific thing is nice idea I think (that pass is enabled only with -Wxxxx flag).

[xbolva00]

Added some tests

[xbolva00]

Added it for msvc too.

[xbolva00]

Hm,
Maybe we have a bug in LLVM's TLI? TLI says Win32 has no memccpy.
https://llvm.org/doxygen/TargetLibraryInfo_8cpp_source.html line 333

cc @RKSimon since he knows windows-related things more.

[xbolva00]

Current solution does not work
/home/xbolva00/LLVM/llvm/tools/clang/include/clang/Basic/Builtins.h:50:34: error: redefinition of ‘BImemccpy’
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,

^

/home/xbolva00/LLVM/llvm/tools/clang/include/clang/Basic/Builtins.h:50:34: note: in definition of macro ‘BUILTIN’
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,

@aaron.ballman, maybe you can help me how to represent that memccpy exists for GNU and MS?

[xbolva00]

This comment was removed by xbolva00.

[xbolva00]

(still interested in this patch)

[xbolva00]

Ping @aaron.ballman

[aaron.ballman]

In D68377#1698399, @xbolva00 wrote:

Current solution does not work
/home/xbolva00/LLVM/llvm/tools/clang/include/clang/Basic/Builtins.h:50:34: error: redefinition of ‘BImemccpy’
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,

^

/home/xbolva00/LLVM/llvm/tools/clang/include/clang/Basic/Builtins.h:50:34: note: in definition of macro ‘BUILTIN’
#define BUILTIN(ID, TYPE, ATTRS) BI##ID,

@aaron.ballman, maybe you can help me how to represent that memccpy exists for GNU and MS?

We may not have the machinery needed for doing this yet, in which case, I think it's fine to ignore the MS builtin.

[xbolva00]

Addressed review note.

[xbolva00]

Added new testcase

[aaron.ballman]

LGTM aside from a minor testing nit.

clang/test/CodeGen/memccpy-libcall.c

13

Please add the newline to the end of the file.

[xbolva00]

Newline.

clang/include/clang/Basic/Builtins.def

 LIBBUILTIN(longjmp, "vJi",        "fr",    "setjmp.h", ALL_LANGUAGES)
 
 // 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(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/test/CodeGen/builtin-memfns.c

 // RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm < %s| FileCheck %s
 
 typedef __WCHAR_TYPE__ wchar_t;
 typedef __SIZE_TYPE__ size_t;
 
 void *memcpy(void *, void const *, size_t);
+void *memccpy(void *, void const *, int, size_t);
 
 // CHECK: @test1
 // CHECK: call void @llvm.memset.p0i8.i32
 // CHECK: call void @llvm.memset.p0i8.i32
 // CHECK: call void @llvm.memcpy.p0i8.p0i8.i32
 // CHECK: call void @llvm.memmove.p0i8.p0i8.i32
 // CHECK-NOT: __builtin
 // CHECK: ret

 
 // CHECK-LABEL: @test12
 extern char dest_array[];
 extern char src_array[];
 void test12() {
   // CHECK: call void @llvm.memcpy{{.*}}(
   memcpy(&dest_array, &dest_array, 2);
 }
+
+// CHECK-LABEL: @test13
+void test13(char *d, char *s, int c, size_t n) {
+  // CHECK: call i8* @memccpy
+  memccpy(d, s, c, n);
+}

clang/test/CodeGen/memccpy-libcall.c

+// RUN: %clang_cc1 -fno-builtin-memccpy -emit-llvm < %s| FileCheck %s
+
+typedef __SIZE_TYPE__ size_t;
+
+void *memccpy(void *, void const *, int, size_t);
+
+void test13(char *d, char *s, int c, size_t n) {
+  // CHECK: call i8* @memccpy{{.*}} #2
+  memccpy(d, s, c, n);
+}
+
+// CHECK: attributes #2 = { nobuiltin }
 No newline at end of file