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

[Siva Chandra via Phabricator via libc-commits]

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


================
Comment at: libc/config/linux/api.td:34
+  let Defn = [{
+    int *__errno_location() __attribute__((const));
+    #define errno *__errno_location()
----------------
abrachet wrote:
> I don't have a Windows machine to test this, but from godbolt, `__attribute__`'s seem to be unrecognized by MSVC or at least const was not a known one. I see other libcs conditionally defining (on `__GNUC__` according to @MaskRay) an `__attribute_const` or something like that.
> 
> FWIW I don't favor adding this attribute. How useful will this even be really?
We are adding the attribute in a Linux only file so should not be a problem because Clang and GCC on Linux support on this attribute?


================
Comment at: libc/src/errno/llvmlibc_errno.h:14
+// public header.
+extern int &llvmlibc_errno;
+
----------------
MaskRay wrote:
> sivachandra wrote:
> > abrachet wrote:
> > > MaskRay wrote:
> > > > I don't think this will work. In the test translation unit, the compiler does not know that `llvmlibc_errno` is bound to a thread-local variable.
> > > > 
> > > > Even in the definition site, it's not clear to me that a reference to a thread-local variable will guarantee to work.
> > > Right, it must be `thread_local int &`. I've tested it only briefly, if the reference isn't `thread_local` it doesn't work. As far as I can tell `thread_local int &` works.
> > > 
> > > 
> > Sigh yes. This declaration has to include `thread_local` for everything to work correctly. Will update in the next round.
> But why do we need a thread_local reference in the first place? It is represented as a thread_local pointer that is initialized to the return address of a TLS wrapper function under the hood. You can check the codegen. The performance is bad.
I did not consider codegen. So yes, the macro wins over the reference if you look at the codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094