[libc-commits] [PATCH] D71094: [libc] Add implementation of errno and define the other macros of errno.h.

Tue Dec 10 09:14:11 PST 2019

sivachandra marked 4 inline comments as done.
sivachandra added inline comments.

================
Comment at: libc/config/linux/api.td:106
+
+    // POSIX
+    SimpleMacroDef<"EPERM", "4">,
----------------
theraven wrote:
> abrachet wrote:
> > MaskRay wrote:
> > > This comment may give a false impression that these constants are defined by POSIX but they are not.
> > > 
> > > On FreeBSD, the values are entirely different.
> > > 
> > > Apparently this will soon be a Linux only project after more Linux specific things are unconsciously baked in.
> > Like above, this is just the Linux config file, thee FreeBSD one will have different values.
> I'm also somewhat concerned at the lack of layering here.  Other parts of libc depend heavily on the definition of errno and on many of these values. 
> 
> If I understand correctly, the list in the POSIX spec file enumerates all of the ones that are required for POSIX, so we should be able to report any missing ones.  It's not clear to me how the two files are layered so that targeting POSIX on Linux will hide the Linux-only definitions.
> 
> The `#include` of the Linux file also makes it difficult to determine when compiling on Linux whether other code is accidentally depending on non-POSIX values.  Ideally, if we're going to include the Linux file, we'd conditionally `#undef` the values that aren't part of POSIX when targeting POSIX.
About being able to check the missing ones:
There is no way currently. However, I plan to do this sometime soon: along with the generated header, generate a smoke test for it. This smoke test will include the generated header and verify if all the items are present in it. There are many details here which are to be discussed, so I will start a thread on libc-dev when I am ready. Note that we should also account for the case wherein a particular platfrom config wants only a part of Linux and a part of POSIX.

About hiding Linux only definitions on a POSIX platform, or vice-versa:
The inclusion of the file `linux/errno.h` has been added in a Linux only config. This Linux only config is supposed to be the stock Linux config where both POSIX and Linux are available. If one wants a Linux-but-no-POSIX config, then it has to be written in a different file. For example, the conditional `#undef`s can go into that platform's `errno.h.inc` file.

Did I understand and address everything your brought up here?

================
Comment at: libc/config/linux/api.td:35
+  let Defn = [{
+    int *__errno_location();
+    #define errno (*__errno_location())
----------------
theraven wrote:
> Is there a reason for implementing errno as a function call?  It's only needed on platforms where the compiler doens't support TLS (and those, I would expect, support emutls with clang?).  In FreeBSD libc, we use this formulation for legacy reasons, but we have moved to using TLS for other `errno`-like things.  Why not declare it as `_Thread_local int errno`? (In FreeBSD's `cdefs.h`, we don't define `_Thread_local` in C11 mode (where it's a keyword, in all other modes it's a reserved identifier), to `thread_local` in C++11 mode, and to `__thread` in any other GNU modes.  We could define it to `declspec(thread)` for MSVC and add the dllimport if required.
> 
> C99 permitted this to be the entire definition, C11 requires you to also `#define errno errno`.
You are correct and I going to change this. What you have suggested is present in modern glibc versions and I agree it is the best approach.
I plan is to move to such a pattern eventually.

The reasons I have not done it in this patch is this: 1. I wanted to move on to implementing mmap and friends which require errno. If we have an implementation of errno, it is easy to swap it out later. 2. We do not have a build rule yet to compile data only C++ files. I did not want to block on that. It was a personal choice. But if you tell me it was a bad one, I am OK to go work on the build rules first and make errno a public _Thread_local value.

================
Comment at: libc/config/linux/api.td:113
+  ];
+}
----------------
theraven wrote:
> This file appears to be missing a definition of `errno_t` when compiling for a C11 target
> .  
TBH, only the errno macro was supposed to be in the scope of this patch. I added the other macros in response to a review comment, and because I thought it was straightforward to add them.

================
Comment at: libc/spec/stdc.td:181
+      ],
+      [], // Types
+      []  // Functions
----------------
theraven wrote:
> I believe this is where `errno_t` should live, though it should be conditionally exposed for C11 or later.
Acknowledged. Conditional exposure is still up for discussion which I will bring up separately.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094