Page MenuHomePhabricator

[Builtins] Teach Clang about memccpy
ClosedPublic

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

Details

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
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 updated this revision to Diff 223017.Oct 3 2019, 7:40 AM

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.

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.

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.

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)

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 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
This revision was automatically updated to reflect the committed changes.