This is an archive of the discontinued LLVM Phabricator instance.

Differential D68377

[Builtins] Teach Clang about memccpy
ClosedPublic

Authored by xbolva00 on Oct 3 2019, 1:19 AM.

Details

Reviewers
aaron.ballman
rsmith
Commits
rGdfece0a108a7: [Builtins] Teach Clang about memccpy
Summary

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

Diff Detail

Event Timeline

xbolva00 created this revision.Oct 3 2019, 1:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 1:19 AM
Herald added subscribers: cfe-commits, kristina. · View Herald Transcript
xbolva00 added a reviewer: rsmith.Oct 3 2019, 1:28 AM
xbolva00 marked an inline comment as done.
xbolva00 added inline comments.
include/clang/Basic/Builtins.def
903 ↗(On Diff #222975)

Well, technically, C 20.

xbolva00 mentioned this in D67986: [InstCombine] snprintf (d, size, "%s", s) -> memccpy (d, s, '\0', size - 1), d[size - 1] = 0.Oct 3 2019, 5:20 AM
xbolva00 updated this revision to Diff 223017.Oct 3 2019, 7:40 AM
aaron.ballman added a comment.Oct 3 2019, 12:31 PM

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 marked an inline comment as done.EditedOct 3 2019, 12:50 PM

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 added a comment.Oct 3 2019, 1:07 PM
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 marked an inline comment as done.Oct 3 2019, 3:06 PM
xbolva00 added inline comments.
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 added inline comments.Oct 4 2019, 11:36 AM
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 marked an inline comment as done.Oct 4 2019, 12:24 PM
xbolva00 added inline comments.
include/clang/Basic/Builtins.def
983 ↗(On Diff #223017)

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

aaron.ballman added inline comments.Oct 4 2019, 12:31 PM
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 marked an inline comment as done.Oct 4 2019, 12:35 PM
xbolva00 added inline comments.
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 added inline comments.Oct 4 2019, 12:39 PM
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 edited the summary of this revision. (Show Details)Oct 4 2019, 12:39 PM
xbolva00 marked an inline comment as done.
xbolva00 added inline comments.
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 marked an inline comment as done.Oct 4 2019, 12:52 PM
xbolva00 added inline comments.
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 added inline comments.Oct 4 2019, 12:57 PM
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 marked an inline comment as done.Oct 4 2019, 1:11 PM
xbolva00 added inline comments.
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 updated this revision to Diff 223440.Oct 6 2019, 12:27 PM

Added some tests

xbolva00 updated this revision to Diff 223441.Oct 6 2019, 12:37 PM

Added it for msvc too.

xbolva00 added a subscriber: RKSimon.Oct 6 2019, 12:40 PM

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 added a comment.Oct 7 2019, 3:25 PM

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 abandoned this revision.Oct 9 2019, 11:08 PM
This comment was removed by xbolva00.
xbolva00 reclaimed this revision.Oct 10 2019, 10:42 AM

(still interested in this patch)

xbolva00 added a comment.Oct 17 2019, 5:25 AM

Ping @aaron.ballman

aaron.ballman added a comment.Oct 17 2019, 8:32 AM
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 updated this revision to Diff 226570.Oct 27 2019, 7:58 AM

Addressed review note.

xbolva00 updated this revision to Diff 226571.Oct 27 2019, 7:59 AM

Added new testcase

Harbormaster completed remote builds in B40100: Diff 226570.Oct 27 2019, 8:00 AM
Harbormaster completed remote builds in B40101: Diff 226571.
aaron.ballman accepted this revision.Oct 28 2019, 9:37 AM

LGTM aside from a minor testing nit.

clang/test/CodeGen/memccpy-libcall.c
13

Please add the newline to the end of the file.

This revision is now accepted and ready to land.Oct 28 2019, 9:37 AM
xbolva00 updated this revision to Diff 226761.Oct 28 2019, 3:24 PM

Newline.

Harbormaster completed remote builds in B40165: Diff 226761.Oct 28 2019, 3:24 PM
Closed by commit rGdfece0a108a7: [Builtins] Teach Clang about memccpy (authored by xbolva00). · Explain WhyOct 28 2019, 3:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
clang/
include/
clang/
Basic/
1 line
test/
CodeGen/
7 lines
12 lines

Diff 226768

clang/include/clang/Basic/Builtins.def

Show First 20 Lines • Show All 973 Lines • ▼ Show 20 Lines
LIBBUILTIN(longjmp, "vJi", "fr", "setjmp.h", ALL_LANGUAGES) LIBBUILTIN(longjmp, "vJi", "fr", "setjmp.h", ALL_LANGUAGES)
// Non-C library functions, active in GNU mode only. // Non-C library functions, active in GNU mode only.
// Functions with (returns_twice) attribute (marked as "j") are still active in // Functions with (returns_twice) attribute (marked as "j") are still active in
// all languages, because losing this attribute would result in miscompilation // all languages, because losing this attribute would result in miscompilation
// when these functions are used in non-GNU mode. PR16138. // when these functions are used in non-GNU mode. PR16138.
LIBBUILTIN(alloca, "v*z", "f", "stdlib.h", ALL_GNU_LANGUAGES) LIBBUILTIN(alloca, "v*z", "f", "stdlib.h", ALL_GNU_LANGUAGES)
// POSIX string.h // 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(stpcpy, "c*c*cC*", "f", "string.h", ALL_GNU_LANGUAGES)
LIBBUILTIN(stpncpy, "c*c*cC*z", "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(strdup, "c*cC*", "f", "string.h", ALL_GNU_LANGUAGES)
LIBBUILTIN(strndup, "c*cC*z", "f", "string.h", ALL_GNU_LANGUAGES) LIBBUILTIN(strndup, "c*cC*z", "f", "string.h", ALL_GNU_LANGUAGES)
// POSIX strings.h // POSIX strings.h
LIBBUILTIN(index, "c*cC*i", "f", "strings.h", ALL_GNU_LANGUAGES) LIBBUILTIN(index, "c*cC*i", "f", "strings.h", ALL_GNU_LANGUAGES)
LIBBUILTIN(rindex, "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) LIBBUILTIN(bzero, "vv*z", "f", "strings.h", ALL_GNU_LANGUAGES)
▲ Show 20 Lines • Show All 577 Lines • Show Last 20 Lines

clang/test/CodeGen/builtin-memfns.c

// RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm < %s| FileCheck %s // RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm < %s| FileCheck %s
typedef __WCHAR_TYPE__ wchar_t; typedef __WCHAR_TYPE__ wchar_t;
typedef __SIZE_TYPE__ size_t; typedef __SIZE_TYPE__ size_t;
void *memcpy(void *, void const *, size_t); void *memcpy(void *, void const *, size_t);
void *memccpy(void *, void const *, int, size_t);
// CHECK: @test1 // CHECK: @test1
// CHECK: call void @llvm.memset.p0i8.i32 // CHECK: call void @llvm.memset.p0i8.i32
// 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.memcpy.p0i8.p0i8.i32
// CHECK: call void @llvm.memmove.p0i8.p0i8.i32 // CHECK: call void @llvm.memmove.p0i8.p0i8.i32
// CHECK-NOT: __builtin // CHECK-NOT: __builtin
// CHECK: ret // CHECK: ret
▲ Show 20 Lines • Show All 98 Lines • ▼ Show 20 Lines
// CHECK-LABEL: @test12 // CHECK-LABEL: @test12
extern char dest_array[]; extern char dest_array[];
extern char src_array[]; extern char src_array[];
void test12() { void test12() {
// CHECK: call void @llvm.memcpy{{.*}}( // CHECK: call void @llvm.memcpy{{.*}}(
memcpy(&dest_array, &dest_array, 2); 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

  • This file was added.
// 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 }
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

Please add the newline to the end of the file.

aaron.ballman: Please add the newline to the end of the file.