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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/errno/errno.cpp:15
+
+thread_local int __llvmlibc_test_errno = 0;
----------------
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`.


================
Comment at: libc/src/errno/llvmlibc_errno.h:13
+#ifdef LLVM_LIBC_PUBLIC_PACKAGING
+#include <errno.h>
+#define ACTIVE_ERRNO errno
----------------
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`.


================
Comment at: libc/src/fcntl/linux/openat.cpp:13
 #include "src/__support/common.h"
+#include "src/errno/llvmlibc_errno.h"
 
----------------
mcgrathr wrote:
> I'm not at all clear on why this isn't in src/__support like other internal things.
I have addressed this under the other comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130662