[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;
----------------
mcgrathr wrote:
> 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.
> 
> 
> 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.

If the variable was actually referenced from internal code, declaring it in a namespace would have made sense. But, considering that the only way it is accessed is via the public `errno` macro, it seemed unnecessary to put it in a namespace and then declare it `extern "C"`. If there is a reason why a namespace still makes sense, I can do it separately.

> 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.

Ah, we can put the header file anywhere - I don't think there is a problem. Before I make that change, I have another question: this patch declares the accessor functions `inline`. Is there a problem if they are not inline? Keeping them `inline` does not work everywhere as `errno` is accessed from places which are not compiled differently for tests. [That is why I said that in a follow up patch I would like to switch this over to an entrypoint target so that it gets compiled differently for tests.]



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130662