[libc-commits] [PATCH] D130662: [libc] Read and write errno via special getter and setter functions.

[Roland McGrath via Phabricator via libc-commits]

mcgrathr added inline comments.


================
Comment at: libc/src/errno/errno.cpp:15
+
+thread_local int __llvmlibc_test_errno = 0;
----------------
sivachandra wrote:
> lntue wrote:
> > mcgrathr wrote:
> > > IMHO this doesn't really belong in the same directory as the "real" errno, let alone the same file.
> > > 
> > > I don't understand why these variables aren't inside `namespace __llvm_libc` like other internal symbols.
> > > 
> > I agree, at least based on its name, some test util is a better place for it.  Also, is the test_errno only used for some tests or for all the tests?
> * About why they are not in the `namespace __llvm_libc`:
> With respect to at least `__llvmlibc_errno`, it is the actual errno which gets used when LLVM libc is the only libc. In the public `errno.h`, we declare it as:
> 
> ```
> extern _Thread_local int __llvmlibc_errno;
> #define errno __llvmlibc_errno
> ```
> 
> For C++ compilations, `_Thread_local` gets replaced with `thread_local`. Because we list `__llvmlibc_errno` directly in the public C header, we cannot declare it in a namespace.
> 
> * About why the "test" flavor belongs in the same directory as the real `errno`:
> I don't think there is any technical blocker preventing us from moving this to `__support` as you have pointed out below. This patch only "extends" the current practices. I suppose it does not fit the way in which Fuchsia intends to use it?  Moving this to `__support` would mean more work in CMake plumbing et al for a few reasons: We don't compile all object files twice, only the ones containing the entrypoints are compiled twice - once for internal consumption, and another time for prod packaging. My idea was to convert the target for this file to an entrypoint target in a follow up patch. If we are to move the test `errno` to a non-entrypoint target in `__support`, we will either have to do some special casing for the `errno` targets, or link in the test `errno` object file into all tests. The latter is much more straight forward to do compared to special casing `errno` in CMake.
> 
> * Is the test `errno` used for some tests or all tests?
> After all usage sites are updated to use the getter and setter functions, all tests will use the test `errno`.
I think the normal way to do this is to declare it inside the namespace but using `extern "C"`.  That way, it can be referred to by the scoped name as well.  If you want to make the scoped name nicer rather than the `__` name, then you can declare it using `__asm__("__name")` instead of using `extern "C"`.  In this case there's no real reason to care about the ergonomics of the variable name, since it is meant to be private to the accessor implementation anyway.

My comment about using `__support` was about the placement of the header file.  I don't think any of the cmake issues about how many ways to compile which source files have direct bearing on where you place a header file.  If we don't mind the COMDAT load, we could also just using an `inline` variable declaration in the header and not have any source file to define it at all.  At least for the one used only in test mode, that doesn't seem like a problem to me.




================
Comment at: libc/src/errno/llvmlibc_errno.h:13
+#ifdef LLVM_LIBC_PUBLIC_PACKAGING
+#include <errno.h>
+#define ACTIVE_ERRNO errno
----------------
sivachandra wrote:
> lntue wrote:
> > Do we have to worry that under the normal testing environments without `LLVM_LIBC_PUBLIC_PACKAGING`, their behavior might diverge due to the inclusion / missing of `errno.h` header?
> Without `LLVM_LIBC_PUBLIC_PACKAGING`, `set_errno` and `get_errno` refer to `__llvmlibc_test_errno`. So, presence or absence of `errno.h` has no impact. My understanding is that that is the desired behavior - when testing, we do not want a mixup with the system libc `errno`.
Any libc implementation code that uses `E*` macros would normally have `#include <errno.h>` by general IWYU principles.   IMHO the API of what headers each source file is required to `#include` to meet IWYU principles should not vary based on how the source is being compiled.  So if the internal API is that implementation files use `#include "src/errno/llvmlibc_errno.h"` in order to implement/use the errno API, then that should consistently either provide the `E*` macros or not. Hence AFAICT it makes most sense to unconditionally `#include <errno.h>` here.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130662