[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:
> 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.]
> 
I think there is merit in the plain uniformity of always using namespaces consistently, that's all.  The basic conventions of the internal API for sources don't need to change to satisfy special cases like different external linkage requirements.

Well, the scheme is kind of fundamentally incompatible with the idea of not compiling code differently for test and production, if you want to keep production optimal by inlining accesses that should be cheaper than a call in most cases.  For Fuchsia's build, everything is compiled differently in the two cases for other reasons anyway so it's not a concern to inline the test-only version in one place and the prod version in the other.  If you really want to avoid it, we can make things macro-parameterized in the header so that your common-to-test-and-prod mode doesn't inline but my test mode and my prod mode do.

OTOH, AIUI the code you compile in common is exactly *not* the public entrypoint code.  There's a fair argument to be made that as `errno` is part of the public API, only the actual public entrypoint functions directly (or their local helpers implemented with them, not in shared places) should actually set errno in any fashion.  So if we make sure that __support et al e.g. return errno values instead of setting the global--which is a generally better style for internal & new APIs anyway--then the errno accessor functions can be reserved for use in the code that *does* get compiled separately for test and prod.

Frankly I'm skeptical that not compiling things multiple times in multiple ways is going to remain an option forever.  There's just too much different about the ways you really might want to compile code for test (e.g. heavy sanitizers) than for production uses.  I'm no cmake wizard, but it doesn't seem likely very difficult to maintain in a well-controlled build like the still-nascent llvm-libc cmake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130662