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

[Siva Chandra via Phabricator via llvm-commits]

sivachandra added a comment.

In D67867#1685213 <https://reviews.llvm.org/D67867#1685213>, @MaskRay wrote:

> I get a bunch of Ninja targets. When the libc is ready, we will get thousands of targets like these:
>
>   build strcat: phony projects/libc/src/string/strcat/strcat
>   build strcat_objects: phony projects/libc/src/string/strcat/strcat_objects
>   build strcat_test: phony bin/strcat_test
>   build strcpy: phony projects/libc/src/string/strcpy/strcpy
>   build strcpy_objects: phony projects/libc/src/string/strcpy/strcpy_objects
>   build strcpy_test: phony bin/strcpy_test
>   build string_h: phony projects/libc/include/string_h
>
>
> Is there some way not to create the targets in the "global namespace"? The target name `bin/strcpy_test` is also less ideal. With thousands tests, those executables can "overflow" people's bin/ directories.


I am not aware of a way to avoid creating targets in the global namespace. But, the target names are restricted C names anyway. So, I am not sure it will be a problem. If it does turn out to be a problem, it should be straightforward to add some prefixes and formulate naming rules as required.

I have now moved the test executables out of the bin directory. But note that the test executables are excluded from the "all" target anyway.

> The build system concerns me the most. I think you should probably find some capable CMake reviewers (I am not well versed in it).

This is the first patch for the libc, and I am sure I did not get everything perfect. So, I also share your concerns about not getting everything perfect. At the same time, I am not worried about this as it isn't like something is getting set in stone - we can change as required as we learn along the way.



================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:27
+    OUTPUT ${dest_file}
+    COMMAND cp ${src_file} ${dest_file}
+    DEPENDS ${src_file}
----------------
MaskRay wrote:
> Use `file(COPY ...)`
file and configure_file do not create concrete build targets. We want concrete build targets so that we have the ability to pick and choose targets.


================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:157
+    OUTPUT ${object_file}
+    DEPENDS ${object_file_raw} llvm-objcopy
+    COMMAND $<TARGET_FILE:llvm-objcopy> --add-symbol "${target_name}=.llvm.libc.entrypoint.${target_name}:0,function" ${object_file_raw} ${object_file}
----------------
MaskRay wrote:
> Prefer CMAKE_OBJCOPY. Does this target need llvm-objcopy to build?
We don't need llvm-objcopy just yet. But, I have a follow up change which will require llvm-objcopy. So, I have been carrying it here as well. Removed now anyway; will put it back in the next change.


================
Comment at: libc/docs/build_system.md:19
+
+Standards like POSIX require that a libc provide certain library files like
+`libc.a`, `libm.a`, etc. The targets for such library files are listed in the
----------------
MaskRay wrote:
> I am not sure names `libm.a` and `libc.a`are required by POSIX or other standards. I think this is more of a convention. Citation needed if you state this.
I couldn't pull out an official reference, but I have this for now: http://www.musl-libc.org/faq.html - See the question "Why is libm.a empty?"


================
Comment at: libc/docs/source_layout.md:70
+
+## The `testing` directory
+
----------------
MaskRay wrote:
> This is usually named `utils/` in LLVM projects.
> 
> `test/` and `unittests/` contain lit tests and unit tests, respectively.
I am for reducing the number of toplevel directories. So, I have modified it to this structure:


    + utils
          build_scripts
          testing



================
Comment at: libc/src/string/strcat/strcat.h:15
+
+char *strcat(char *dest, const char *src);
+
----------------
MaskRay wrote:
> delete dest and src
This is an internal header file, so I do not see any problem with keeping the names.


================
Comment at: libc/src/string/strcpy/strcpy.h:15
+
+char *strcpy(char *dest, const char *src);
+
----------------
MaskRay wrote:
> Delete `dest` and `src`.
This is an internal header, so I do not see any problem with keeping the names.


================
Comment at: llvm/CMakeLists.txt:62
 # one for llvm+clang+... using the same sources.
-set(LLVM_ALL_PROJECTS "clang;clang-tools-extra;compiler-rt;debuginfo-tests;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;openmp;parallel-libs;polly;pstl")
+set(LLVM_ALL_PROJECTS "clang;clang-tools-extra;compiler-rt;debuginfo-tests;libc;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;openmp;parallel-libs;polly;pstl")
 set(LLVM_ENABLE_PROJECTS "" CACHE STRING
----------------
MaskRay wrote:
> Making it build by default makes me concerned: there are some cmake constructs which I am not sure build on a non-Linux platform. If nobody has tested that, probably hold off on this a bit.
I am not sure what you mean by "default". libc will be built only if one specifies libc among the list of project with -DLLVM_ENABLE_PROJECTS, or if one says "all".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867