[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:
> MaskRay wrote:
> > sivachandra wrote:
> > > 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?
> > Using unprotected `__attribute__` will break cross compilability. MSVC does not accept it. Or you can say llvm-libc doesn't support MSVC. I'm totally fine with it.
> > 
> > > FWIW I don't favor adding this attribute. How useful will this even be really?
> > 
> > This matters when you run lots of functions that may potentially modify `errno` in a function.
> > 
> > ```
> > foo();
> > if (errno)
> >   ...
> > foo();
> > if (errno)
> >   ...
> > foo();
> > if (errno)
> >   ...
> > ```
> > Using unprotected __attribute__ will break cross compilability. MSVC does not accept it. Or you can say llvm-libc doesn't support MSVC. I'm totally fine with it.
> Like @sivachandra said earlier, this is just the config file for Linux, if we support Windows later then it will not use this attribute in its config file.
> 
> >This matters when you run lots of functions that may potentially modify `errno` in a function.
> Hmm. I'm not trying to argue, this is purely out of curiosity at this point, because a lot of libcs use this attribute. If I'm understanding correctly, that code will call `__errno_location()` once and use the first value each time after. But can't different calls to `foo` set `errno` differently?
You have a valid point and I have to confess I do not really know how the const attribute works in general. My homework showed me that glibc and musl both use the const attribute and I was not really sure if it is only useful in cases like errno + errno, so I left it out initially. Also, I am of the opinion that adding attributes like this should be driven by some implementation standards backed by some tooling.

As you feel, even I do not want to argue over these things especially when they are correct in way. Considering musl and glibc use the const attribute, I think there is no harm using it but I am open to being proven wrong for accepting this suggestion.


================
Comment at: libc/src/errno/llvmlibc_errno.h:18
+
+int *__errno_location();
+
----------------
MaskRay wrote:
> Mark locally defined libc symbols hidden `__attribute__((visibility("hidden")))` in internal headers. When the library is built with -fPIC (for shared objects), the codegen will not create unnecessary PLT or GOT relocations.
> 
> See musl/src/include/errno.h for an example.
That's a good suggestion. I think its not yet decided if items like these are really internal. I understand musl treats in a particular way and glibc treats it in a slightly different way. In general, I like your suggestion. However,  I would like to keep this open for now as we need to make a decision about what exactly do we mean by internal (For example, we want this libc to work along with other libcs. So, overridability of some form is probably desirable.). After we make a decision, we should probably drive this more cleanly using some set acceptable patterns backed by some tooling to verify adherence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094