aaron.ballman added inline comments.
================ Comment at: include/clang/Basic/Builtins.def:483 BUILTIN(__builtin_memcpy, "v*v*vC*z", "nF") +BUILTIN(__builtin_memccpy, "v*v*vC*iz", "nF") BUILTIN(__builtin_memmove, "v*v*vC*z", "nF") ---------------- xbolva00 wrote: > aaron.ballman wrote: > > xbolva00 wrote: > > > aaron.ballman wrote: > > > > xbolva00 wrote: > > > > > aaron.ballman wrote: > > > > > > GCC doesn't seem to have `__builtin_memccpy`? > > > > > > https://godbolt.org/z/jbthQ3 > > > > > Ok, I will drop it. > > > > 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. > > > 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 > > 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. > Yeah, I am sorry, more context why this is needed: > > https://reviews.llvm.org/D67986 Thank you for the extra context! ================ Comment at: include/clang/Basic/Builtins.def:983 // 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) ---------------- xbolva00 wrote: > aaron.ballman wrote: > > xbolva00 wrote: > > > aaron.ballman wrote: > > > > xbolva00 wrote: > > > > > aaron.ballman wrote: > > > > > > Isn't `memccpy` supported by Visual Studio? What should happen > > > > > > there? > > > > > ”This POSIX function is deprecated. Use the ISO C++ conformant > > > > > _memccpy instead.“ > > > > Thanks for verifying it's still supported (deprecated != unsupported). > > > > We do support other Microsoft builtins, so should this one be added? > > > Ok, I will add it. > > > > > > One last question, should I somehow represent that memccpy is in C 20 (if > > > so, how) ? > > 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?) > >> 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. > 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68377/new/ https://reviews.llvm.org/D68377 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
