[PATCH] D67867: [libc] Add few docs and implementation of strcpy and strcat.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D67867#1686112 <https://reviews.llvm.org/D67867#1686112>, @jyknight wrote:

> In D67867#1686056 <https://reviews.llvm.org/D67867#1686056>, @hfinkel wrote:
>
> > Maybe everything is fine, but given this setup, does anyone see any potential problems with compiling these functions for nvptx? I'd like to eventually see a mode where we compile an appropriate subset of these functions for GPU targets -- either in bitcode form for linking with our device-side OpenMP runtime library or as a native object -- to provide a more feature-complete offloading environment.
> >
> > The one thing that caught by eye was the use of the section attribute, and I was curious what nvptx does with that. As far as I can tell, perhaps the answer is nothing.
>
>
> Then I think this scheme won't work, since the point of the sections is to enable the creation of the global symbols post-build.
>
> E.g., I think the idea is that the main implementation defines the function with C++ name `__llvm_libc::strcpy(char *, const char *)`, and places the code in the `.llvm.libc.entrypoint.strcpy` section. And then another tool comes along and iterates the llvm.libc.entrypoint sections, and adds global symbol aliases for each one.
>
> That scheme feels probably over-complex, IMO, but I don't have an concrete counter-proposal in mind.


Another problem with the `.llvm.libc.entrypoint.strcpy` + `objcopy --add-symbol` scheme is that there is no way to change the st_size field. This can impact symbolization preciseness when debugging information is stripped. This makes me wonder what advantages the namespace `__llvm_libc::` brings. @theraven questioned the disadvantage:

> This means that we can test the namespaced libc functions in an environment that already has a libc, but we can't easily extend these tests to run in a process (or other environment) that has only LLVM libc.



================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:164
+    DEPENDS ${object_file_raw}
+    COMMAND ${CMAKE_OBJCOPY} --add-symbol "${target_name}=.llvm.libc.entrypoint.${target_name}:0,function" ${object_file_raw} ${object_file}
+  )
----------------
`,function` -> `,function,global` otherwise it is STB_LOCAL.


================
Comment at: libc/include/string.h:18
+
+void *memcpy(void *, const void *, size_t);
+
----------------
theraven wrote:
> Missing `restrict` qualifiers on this and many other standard functions in this file.  The standard defines `memcpy` as:
> 
> ```
> void *memcpy(void * restrict s1,
> const void * restrict s2,
> size_t n);
> ```
> 
> I'm surprised that clang doesn't warn about the declaration of `memcpy` with incorrect types - it usually notices missing qualifiers.
So this should be

```
void *memcpy(void *__restrict, const void *__restrict, size_t);
```

to be usable in C++


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67867/new/

https://reviews.llvm.org/D67867