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

[Siva Chandra via Phabricator via llvm-commits]

sivachandra marked 3 inline comments as done.
sivachandra added a comment.

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

> Actually, now that I think I understand the existing proposal better, I believe it's broken, as well as confusing. It's getting the same effect as using `__attribute__((alias))`, except harder to understand. But it's not ok to have a single object file expose both a strong public alias and an internal alias, for any function that's not in the baseline ISO C standard. It would be OK if the aliases were weak, or if they were strong but exposed by a separate .o file. (In any case, I'd like to suggest not using an external objcopy invocation to achieve this.)


The objcopy step is required to avoid putting mangled names with the alias attribute. If there is any other way to achieve the same thing, I am open to it.

> For example of why this is wrong -- consider if libc has an 'open.o' object file, which defines `__llvm_libc::open`, and has also had the alias `open` added to it with objcopy. Internally, if libc needs to call open, it calls `__llvm_libc::open`, which pulls in that open.o file, which then also defines the global 'open' function. Then there thus be a duplicate symbol error for any Standard C (e.g. non-posix) program which defines its own open function.

I am open to making all public symbols weak. Should we start with that, or should make them weak on an as-needed basis?



================
Comment at: libc/docs/header_generation.md:21
+2. Replace the line on which a command occurs with some other text as directed
+by the command. The replacment text can span multiple lines.
+
----------------
jyknight wrote:
> theraven wrote:
> > sivachandra wrote:
> > > theraven wrote:
> > > > This sounds like you will end up with only one set of headers per configuration, so you lose the ability to have different projects using the same generated headers but enforcing different sets of standards compliance in their use of the interfaces.
> > > Yes, that is the general direction in which this is going. We are making the headers for a configuration much simpler to navigate at the cost of having multiple sets of headers. In this day and age, I do not think forcing multiple sets of header files is a bad thing. Note also that users' build systems already have the knowledge and capability to handle multiple configurations. Hence, we are not making the build systems any more complicated.
> > > 
> > > This is not what traditional libcs have done. So, yes we are introducing a "third mechanism". At the same time, one can also argue that we are doing away with such mechanisms as we require that each configuration have its own set of header files.
> > I think that's fine when you consider building libc and shipping a single configuration, but a lot of projects that I've seen have different feature macros defined for different components.  Are they now expected to rebuild the libc headers multiple times for each module?  Do they need to drive that from their own build system (which is often not CMake)?  It's even more complex when a project contains C89, C11, and C++11 files - these all have subtly different sets of requirements for the functions exposed in libc headers: do we require that they build a set for each?  Or do you imagine that anyone shipping C11 will ship a powerset of headers?  
> > 
> > The reason that we don't do the separate header thing in libcs today is that we end up with a huge explosion of the set of things that are supported.  For example, in FreeBSD we support 3 versions of the C standard, 3 or 4 versions of POSIX, GNU and BSD extensions.  Almost any combination of these is allowed, so we're looking at 20-30 possible sets of header files, before we start considering restricted subsets for sandboxed applications, custom configurations for sanitisers, and so on.
> IMO, it makes sense not to bother making C99/C11-only functions conditionally available. The libc headers still ought to be compatible in C89 mode, but I don't see that there's really much point to excluding declarations for new functions like 'strtof', 'aligned_alloc', etc, when building in older standards modes.
> 
> The same most likely can apply to old POSIX versions.
> 
> However, I do think it is quite likely to be necessary to preserve the ability to conditionally disable the various standards "layers". That is -- for all the headers specified in ISO C, you should be able to disable the declarations added by POSIX (and extensions) with a define. And for all the headers specified in POSIX, you should be able to disable the declarations added by the GNU/BSD/etc extensions with a define.
With respect to excluding extension standards, I will go back to my earlier comment here: The %%include mechanism gives us a way to do it, but calls for a header set per configuration. Note that the libc source tree still only has a single set of files.

True that there will be an "explosion of configurations", but I expect a large number of these to be downstream configurations. Off-the-shelf, we should probably only provide what a normal Linux or a Windows development environment needs.


================
Comment at: libc/include/math.h:20
+
+long double acosl(long double);
+
----------------
theraven wrote:
> sivachandra wrote:
> > theraven wrote:
> > > The `long long` functions should be exposed only for C99 or later and a version of C++ that supports the `long long` type.  
> > We are C17 and higher already. Should we still have such conditions?
> We are building as C++17.  There is no C17.  I would hope that we're still aiming for the headers to be consumed by C89 programs, because there are a huge number of those in the world.  
We have said in the proposal that we will only support C17 and higher: http://llvm.org/docs/Proposals/LLVMLibC.html


================
Comment at: libc/include/string.h:16
+#define __need_NULL // To get only NULL from stddef.h
+#include <stddef.h>
+
----------------
jyknight wrote:
> theraven wrote:
> > Does this work correctly with the inclusion guards?  I don't see the `stddef.h` implementation here, so I don't know what those macros do (they don't do anything in FreeBSD libc's `stddef.h`, they appear to do something in libc++'s `cstddef`, though I'm not entirely sure what) .  
> > 
> > The FreeBSD solution so this is to define types like `__size_t`, use these in headers that are supposed to use `size_t` in function prototypes but not provide a definition of `size_t` (yes, there are several in the C spec, it's annoying but that's what the standard says), and then add a guarded typedef to turn that into `size_t` in this header, `stddef.h` and a couple of other places.
> Both Clang and GCC ship a stddef.h which supports these defines -- and it's expected to be first in the include path, before libc's headers.
> 
> For some reason, freebsd and some other platforms remove these compiler-shipped files and replace them with their own for their libc.
> 
> Exactly what the contract should be between the compiler headers and the libc headers could be a larger discussion, but for now, I'm strongly in favor of assuming that we're using the existing stddef.h from clang/gcc -- in which case this code will work correctly.
I was convinced by jyknight's reasoning last time. If not anything else, I like that it keeps our implementation surface smaller.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867