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

[David Chisnall via Phabricator via libc-commits]

theraven added inline comments.


================
Comment at: libc/config/linux/api.td:106
+
+    // POSIX
+    SimpleMacroDef<"EPERM", "4">,
----------------
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.


================
Comment at: libc/config/linux/api.td:35
+  let Defn = [{
+    int *__errno_location();
+    #define errno (*__errno_location())
----------------
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`.


================
Comment at: libc/config/linux/api.td:113
+  ];
+}
----------------
This file appears to be missing a definition of `errno_t` when compiling for a C11 target
.  


================
Comment at: libc/spec/stdc.td:181
+      ],
+      [], // Types
+      []  // Functions
----------------
I believe this is where `errno_t` should live, though it should be conditionally exposed for C11 or later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094