[libc] Add implementation of errno and define the other macros of errno.h.

[abrachet]

libc/config/linux/api.td

35

We don't use extern for other function definitions in other header files so I don't think we should here

libc/spec/stdc.td

176

Are adding the errno macros out of the scope of this patch?

libc/src/errno/errno_test.cpp

14

Something like Basic seems more conventional than Smoke for a test name. What is it supposed to mean?

libc/src/errno/llvmlibc_errno.h

13

Internally we could use a thread_local int & instead of relying on a macro, I believe. It seems nicer, what do you think?

[MaskRay]

libc/config/linux/api.td

36

()

Otherwise errno[0] is incorrect.

[MaskRay]

libc/config/linux/api.td

35

You need __attribute__((const)) in __GNUC__ mode, otherwise common subexpression elimination does not apply: think errno+errno.

[sivachandra]

Address comments.

[sivachandra]

libc/config/linux/api.td

36

We don't want to put it in parenthesis because we want to error on uses like errno[0]. errno is supposed to be an lvalue of int type and allowing errno[0] actually amounts to leaking implementation detail.

libc/spec/stdc.td

176

The generated header is not coming out pretty (though syntactically correct) with a large number of macros added (the order seems messed up ). I had wanted to first fix that up and then add the remaining macros. But, after reading your comment, it occurs to me that it is better to fix the problem than to hedge it. So, I have added the remaining macros now. Will improve the look of the generated file with a different patch.

libc/src/errno/errno_test.cpp

14

Smoke for https://en.wikipedia.org/wiki/Smoke_testing_(software).
I have changed it to Basic now anyway.

libc/src/errno/llvmlibc_errno.h

13

That's a good idea so I have done it now.

[sivachandra]

libc/spec/stdc.td

176

Also, I am not sure if a libc should provide these macros or if we should fallback to the ones provided by the Linux headers. Glibc seems to be preferring to use the Linux headers, while musl seems to be providing them itself. May be @stanshebs can throw some light here.

[sivachandra]

libc/spec/stdc.td

176

I took a closer look at the numbers used by musl. They are just a copy of the linux numbers. Also, ISTM that if Linux provides them, we should use them. Will discuss with @stanshebs and update the patch.

[MaskRay]

libc/config/linux/api.td

36

I know errno[0] will be an error anyway.

However, this breaks grouping: *__errno_location()[0]

You also don't want 3 errno to be treated as 3*__errno_location(), do you?

libc/src/errno/llvmlibc_errno.h

15

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.

[abrachet]

libc/src/errno/errno_location.cpp

15

If we do end up keeping this reference we could also put it behind namespace __llvm_libc and call it something less verbose, perhaps.

libc/src/errno/llvmlibc_errno.h

15

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.

[sivachandra]

libc/src/errno/llvmlibc_errno.h

15

Sigh yes. This declaration has to include thread_local for everything to work correctly. Will update in the next round.

[sivachandra]

libc/src/errno/errno_location.cpp

15

I did consider __llvm_libc::errno. But, now since errno is a token by itself, it interferes with the public macro errno when used along with other libraries, and even gtest.
Your comment now points me to the fact that the name should actually be __llvmlibc_errno to avoid name pollution. If you have a suggestion for a shorter name within the namespace __llvm_libc which can be less verbose than __llvmlibc_errno, I will take it.

Will update in the next round.

[MaskRay]

libc/src/errno/llvmlibc_errno.h

15

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.

[sivachandra]

libc/config/linux/api.td

36

To be clear here, I do not really have an opinion and I am OK to put it in parenthesis. Other libcs have chosen not to enclose in parenthesis which I thought was to prevent some form of abuse. But, I do see your point that either way it can be abused. If @stanshebs does not tell us anything interesting, I will put it in parenthesis.

[abrachet]

libc/config/linux/api.td

35

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?

[sivachandra]

libc/config/linux/api.td

35

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?

libc/src/errno/llvmlibc_errno.h

15

I did not consider codegen. So yes, the macro wins over the reference if you look at the codegen.

[abrachet]

libc/src/errno/llvmlibc_errno.h

15

You can check the codegen. The performance is bad.

Yup I didn't realize it would be much worse. I agree that it is not worth the performance overhead and we should just go back to the macro.

[sivachandra]

Address @MaskRay and @alexbrachet's comments.

[MaskRay]

For posterity, #define errno *__errno_location() breaks C11 7.1.2p5:

Any definition of an object-like macro described in this clause shall expand to code that is fully protected by parentheses where necessary, so that it groups in an arbitrary expression as if it were a single identifier.

It will fail to diagnose code like 3 errno[0] (false positive), and incorrectly flag (false negative):

const char *blah[] = { [EILSEQ] = "asdf" };
errno = EILSEQ;
puts(errno[blah]);

and errno++

libc/config/linux/api.td

35

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)
  ...

[MaskRay]

libc/config/linux/api.td

107

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.

[abrachet]

libc/config/linux/api.td

35

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?

107

Like above, this is just the Linux config file, thee FreeBSD one will have different values.

[sivachandra]

Instead of defining the error macros, use the ones available in linux/errno.h

[sivachandra]

In D71094#1773028, @MaskRay wrote:

For posterity, #define errno *__errno_location() breaks C11 7.1.2p5:

Any definition of an object-like macro described in this clause shall expand to code that is fully protected by parentheses where necessary, so that it groups in an arbitrary expression as if it were a single identifier.

It will fail to diagnose code like 3 errno[0] (false positive), and incorrectly flag (false negative):

const char *blah[] = { [EILSEQ] = "asdf" };
errno = EILSEQ;
puts(errno[blah]);

and errno++

FWIW, I had accepted your suggestion before you made this comment.

[stanshebs]

libc/spec/posix.td

2

Is this expected to be definitive for what POSIX specifies? EHWPOISON is Linux-only I think.

libc/src/errno/errno_location.cpp

16

One bit of handiness that glibc does is to alias it its version of __errno with plain "errno", so you see it as "errno" in a debugger, object file dumps, etc. (In theory, getting errno through macro expansion and function call should be possible in debugger, but is much less straightforward and more likely to fail, as with core dumps.)

[sivachandra]

Split the error macros in two POSIX and Linux based on https://pubs.opengroup.org/onlinepubs/9699919799/

[sivachandra]

libc/spec/posix.td

2

I do not know if https://pubs.opengroup.org/onlinepubs/9699919799/ is a definitive guide, but I have now split this list up into POSIX and Linux based on that link.

libc/src/errno/errno_location.cpp

16

Aliases in C++ code will require mangled names. One can use the same section trick, but prefer to do it in a later pass iff required.

[MaskRay]

libc/src/errno/llvmlibc_errno.h

19

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.

[sivachandra]

libc/config/linux/api.td

35

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.

libc/src/errno/llvmlibc_errno.h

19

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.

[abrachet]

libc/config/linux/api.td

36

Remove the ;

99

'depend on the linux/errno.h' -> 'depend on linux/errno.h'

libc/include/errno.h.def

15

glibc has an ifdef block around the C definitions to exclude them if ASSEMBLER is defined so that only the E* macros are exposed to assembly files which want them. I would imagine no one is including C's errno.h instead of linux/errno.hin assembly source, but I figured I would bring it up. https://code.woboq.org/userspace/glibc/stdlib/errno.h.html#32

[sivachandra]

Address abrachet's comments.

[sivachandra]

libc/include/errno.h.def

15

I prefer to add anything like that on an "as needed" basis.

[MaskRay]

errno is a fundamental component baked in almost every component of a libc and contributors/reviewers should really be on the same page. Both abrachet and stanshebs accepted the patch and they were the initially-added reviewers. I feel a bit lonely to be the only one in the "Request Changes" camp, but I hope you do not commit the patch with these open issues not resolved. To reiterate, I feel strong about my suggested items:

• __attribute__((visibility("hidden"))) in the internal header.
• __attribute__((const)) in the public header.

They are adopted by both glibc and musl.

You are reluctant to add hidden visibility because you feel that it may make overlaying llvm-libc on top of system libc harder? Can you discuss more about the details somewhere, e.g. libc-dev or llvm-dev?

[MaskRay]

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

[sivachandra]

In D71094#1775667, @MaskRay wrote:

errno is a fundamental component baked in almost every component of a libc and contributors/reviewers should really be on the same page. Both abrachet and stanshebs accepted the patch and they were the initially-added reviewers. I feel a bit lonely to be the only one in the "Request Changes" camp, but I hope you do not commit the patch with these open issues not resolved. To reiterate, I feel strong about my suggested items:

• __attribute__((visibility("hidden"))) in the internal header.
• __attribute__((const)) in the public header.

To be clarify here, I have added the const attribute as soon as I have seen your suggestion.

They are adopted by both glibc and musl.

You are reluctant to add hidden visibility because you feel that it may make overlaying llvm-libc on top of system libc harder? Can you discuss more about the details somewhere, e.g. libc-dev or llvm-dev?

As I have said previously, we want to add visibility attributes in a more principled manner. I have also said previously that your suggestion is very valid, and that it was considered even before putting this patch out.

A discussion on libc-dev or llvm-dev will definitely be started when I am prepared with all the details I want to bring up. Or, if someone wants to champion the whole area of interop with other libcs, I would be glad to lean on their help. I do not see why such a discussion should block this patch. FWIW, this patch helps me add more of the library, which I think is of greater importance to keep the development of this library running.

[sivachandra]

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

I appreciate that you and @abrachet have volunteered to do the reviews here. At the same time, I find the tone of the above comment unfriendly. AFAICT, @abrachet is doing this voluntarily out of his own interest. An unfriendly tone for actions, which I view as "friendly community citizenship", definitely is uncalled for.

[stanshebs]

In D71094#1775667, @MaskRay wrote:

errno is a fundamental component baked in almost every component of a libc and contributors/reviewers should really be on the same page. Both abrachet and stanshebs accepted the patch and they were the initially-added reviewers. I feel a bit lonely to be the only one in the "Request Changes" camp, but I hope you do not commit the patch with these open issues not resolved. To reiterate, I feel strong about my suggested items:

• __attribute__((visibility("hidden"))) in the internal header.
• __attribute__((const)) in the public header.

They are adopted by both glibc and musl.

You are reluctant to add hidden visibility because you feel that it may make overlaying llvm-libc on top of system libc harder? Can you discuss more about the details somewhere, e.g. libc-dev or llvm-dev?

My reason to accept this now is that secondary issues like visibility are almost certainly going to need revisiting later, when we have more content to work with. For instance, do we do it by adding attributes everywhere in sight, or do we build some kind of script or other automation? Given that this is being developed incrementally from zero, we are just making extra unnecessary work by insisting that the defacto prototype solve all the problems that might need solving in the future. So let's get the core of the system figured out now, and not worry about secondary issues until more of the system is in place.

[abrachet]

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

Sorry about that there ended up being many more changes than I anticipated here when I accepted. I'll be more careful next time.

In D71094#1775694, @sivachandra wrote:

As I have said previously, we want to add visibility attributes in a more principled manner. I have also said previously that your suggestion is very valid, and that it was considered even before putting this patch out.

A discussion on libc-dev or llvm-dev will definitely be started when I am prepared with all the details I want to bring up. Or, if someone wants to champion the whole area of interop with other libcs, I would be glad to lean on their help. I do not see why such a discussion should block this patch. FWIW, this patch helps me add more of the library, which I think is of greater importance to keep the development of this library running.

I think @sivachandra and I are on the same page here that things are bound to change later and currently the ground work is being laid so that development can truly start. I think its worth starting a thread on the libc list of how reviews should go and what we have in mind for these current revisions.

In D71094#1775720, @sivachandra wrote:

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

I appreciate that you and @abrachet have volunteered to do the reviews here. At the same time, I find the tone of the above comment unfriendly. AFAICT, @abrachet is doing this voluntarily out of his own interest. An unfriendly tone for actions, which I view as "friendly community citizenship", definitely is uncalled for.

FWIW I don't find @MaskRay's comments to be unfriendly, and I value his opinion a lot, he has helped me on my patches quite a bit in the past. He simply pointed out something I could do better, and I'm glad he did.

[sivachandra]

In D71094#1775764, @abrachet wrote:

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

Sorry about that there ended up being many more changes than I anticipated here when I accepted. I'll be more careful next time.

If it can make you feel better, apart from the minor nits, the "major" changes between the latest version and the version you have accepted is the addition of the const attribute and the new error macros. We have explored the idea of making errno a reference for internal use, but @MaskRay thankfully pointed out to the fact that it leads to a lot more code gen.

In D71094#1775694, @sivachandra wrote:

As I have said previously, we want to add visibility attributes in a more principled manner. I have also said previously that your suggestion is very valid, and that it was considered even before putting this patch out.

A discussion on libc-dev or llvm-dev will definitely be started when I am prepared with all the details I want to bring up. Or, if someone wants to champion the whole area of interop with other libcs, I would be glad to lean on their help. I do not see why such a discussion should block this patch. FWIW, this patch helps me add more of the library, which I think is of greater importance to keep the development of this library running.

I think @sivachandra and I are on the same page here that things are bound to change later and currently the ground work is being laid so that development can truly start. I think its worth starting a thread on the libc list of how reviews should go and what we have in mind for these current revisions.

I also would like to point out that I have addressed each every comment on this review. I did not accept one suggestion, but I have backed it up with some reasoning.

For the sake of progress, I will submit this change soon and move on to other patches.

About, "how reviews should go ... " -- We do not want to be different from any other LLVM code review. I have picked reviewers based on who I feel might have the best feedback for a particular change. I would think that it is the normal practice, which for me personally, has been reinforced with my past experience in the LLVM community.
About, "and what we have in mind for these current revisions." -- If I can come up with something worth while, I will definitely post it to list. If a major design decision has to be made, we should of course have a discussion on the dev list. We have done this for the header generation idea, and going forward I am sure we will have many more such discussions. I do not think it is practical to discuss each and every patch on the list.

[MaskRay]

In D71094#1775720, @sivachandra wrote:

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

I appreciate that you and @abrachet have volunteered to do the reviews here. At the same time, I find the tone of the above comment unfriendly. AFAICT, @abrachet is doing this voluntarily out of his own interest. An unfriendly tone for actions, which I view as "friendly community citizenship", definitely is uncalled for.

Apologize if you feel that my comments were not friendly. I did not intend that.

My reason to accept this now is that secondary issues like visibility are almost certainly going to need revisiting later, when we have more content to work with. For instance, do we do it by adding attributes everywhere in sight, or do we build some kind of script or other automation? Given that this is being developed incrementally from zero, we are just making extra unnecessary work by insisting that the defacto prototype solve all the problems that might need solving in the future. So let's get the core of the system figured out now, and not worry about secondary issues until more of the system is in place.

My feeling is that making the attribute change part of the patch is preferable. It makes the intention clear what symbols are public and what symbols and internal implementation details. Adding errno in an incremental step looks great to me. But making attribute changes separate is not. From the comments, it seems that __errno_location may be overlayed on top of an existing libc? I am a bit more concerned about the approach that will be used to do the overlay work. The design choice made here needs to account for standalone libc mode + overlay mode. I think we need to make the two modes clear.

[sivachandra]

In D71094#1775911, @MaskRay wrote:

In D71094#1775720, @sivachandra wrote:

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

I appreciate that you and @abrachet have volunteered to do the reviews here. At the same time, I find the tone of the above comment unfriendly. AFAICT, @abrachet is doing this voluntarily out of his own interest. An unfriendly tone for actions, which I view as "friendly community citizenship", definitely is uncalled for.

Apologize if you feel that my comments were not friendly. I did not intend that.

My reason to accept this now is that secondary issues like visibility are almost certainly going to need revisiting later, when we have more content to work with. For instance, do we do it by adding attributes everywhere in sight, or do we build some kind of script or other automation? Given that this is being developed incrementally from zero, we are just making extra unnecessary work by insisting that the defacto prototype solve all the problems that might need solving in the future. So let's get the core of the system figured out now, and not worry about secondary issues until more of the system is in place.

My feeling is that making the attribute change part of the patch is preferable. It makes the intention clear what symbols are public and what symbols and internal implementation details. Adding errno in an incremental step looks great to me. But making attribute changes separate is not. From the comments, it seems that __errno_location may be overlayed on top of an existing libc? I am a bit more concerned about the approach that will be used to do the overlay work. The design choice made here needs to account for standalone libc mode + overlay mode. I think we need to make the two modes clear.

A higher level point I have been trying to make is that the design is open. I have not spent enough time on it to write to any list and make a case. Under such circumstances, we do not want to commit to any particular thing.

[sivachandra]

@theraven has been added as a reviewer. But, I prefer to commit this patch without waiting on his review. If @theraven has any comments, I will be happy to address them post commit.

[theraven]

libc/config/linux/api.td

35

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.

107

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.

113

This file appears to be missing a definition of errno_t when compiling for a C11 target
.

libc/spec/stdc.td

181

I believe this is where errno_t should live, though it should be conditionally exposed for C11 or later.

[sivachandra]

libc/config/linux/api.td

35

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.

107

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 #undefs can go into that platform's errno.h.inc file.

Did I understand and address everything your brought up here?

113

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.

libc/spec/stdc.td

181

Acknowledged. Conditional exposure is still up for discussion which I will bring up separately.

libc/config/linux/api.td

 include "config/public_api.td"
 
+include "spec/linux.td"
+include "spec/posix.td"
 include "spec/stdc.td"
 
 def FloatT : TypeDecl<"float_t"> {
   let Decl = [{
     #if __FLT_EVAL_METHOD__ == 1
       typedef float float_t
     #elif __FLT_EVAL_METHOD__ == 2
       ...

 
 def NullMacro : MacroDef<"NULL"> {
   let Defn = [{
     #define __need_NULL
     #include <stddef.h>
   }];
 }
 
+def ErrnoMacro : MacroDef<"errno"> {
+  let Defn = [{
+    int *__errno_location();
+    #define errno (*__errno_location())
+  }];
+}
+
 def MathAPI : PublicAPI<"math.h"> {
   let Functions = [
    "acos",
    "acosl",
   ];
 
   let TypeDeclarations = [
     FloatT,
def StdIOAPI : PublicAPI<"stdio.h"> {
   let TypeDeclarations = [
     SizeT,
   ];
 
   let Functions = [
     "snprintf",
   ];
 }
+
+def ErrnoAPI : PublicAPI<"errno.h"> {
+  let Macros = [
+    ErrnoMacro,
+    // We largely depend on linux/errno.h to give us the
+    // various error macro definitions. However, some libc
+    // implementations have chosen to provide definitions
+    // for some of the error macros to account for the ones
+    // missing in linux/errno.h. There is no harm in doing
+    // the same here if we define the macros only when they
+    // are not already defined.
+    MacroDefineIfNot<"ENOTSUP", "EOPNOTSUPP">,
+    MacroDefineIfNot<"ECANCELED", "125">,
+    MacroDefineIfNot<"EOWNERDEAD", "130">,
+    MacroDefineIfNot<"ENOTRECOVERABLE", "131">,
+    MacroDefineIfNot<"ERFKILL", "132">,
+    MacroDefineIfNot<"EHWPOISON", "133">,
+  ];
+}

libc/config/linux/errno.h.in

+//===---------------- Linux specific errno.h definitions ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+%%begin()
+
+#include <linux/errno.h>

libc/config/public_api.td

 include "spec/spec.td"
 
 class TypeDecl<string name> {
   string Name = name;
   string Decl = "";
 }
 
 class MacroDef<string name> {
   string Name = name;
   string Defn = "";
 }
 
+class SimpleMacroDef<string name, string value> : MacroDef<name> {
+  let Defn = !strconcat("#define ", name, " ", value);
+}
+
+class MacroDefineIfNot<string name, string value> : MacroDef<name> {
+  let Defn = !strconcat("#ifndef ", name, "\n",
+                        "#define " , name, " ", value, "\n",
+                        "#endif // ", name);
+}
+
 class PublicAPI<string name> {
   string HeaderName = name;
   list<MacroDef> Macros = [];
   list<TypeDecl> TypeDeclarations = [];
   list<string> Structs = [];
   list<string> Functions = [];
 }

libc/include/CMakeLists.txt

 
 add_gen_header(
   string_h
   DEF_FILE string.h.def
   GEN_HDR string.h
   DEPENDS
     llvm_libc_common_h
 )
+
+add_gen_header(
+  errno_h
+  DEF_FILE errno.h.def
+  PARAMS
+    platform_errno=../config/${LIBC_TARGET_OS}/errno.h.in
+  GEN_HDR errno.h
+  DATA_FILES
+    ../config/${LIBC_TARGET_OS}/errno.h.in
+)

libc/include/errno.h.def

+//===---------------- C standard library header errno.h ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_ERRNO_H
+#define LLVM_LIBC_ERRNO_H
+
+#include <__llvm-libc-common.h>
+
+%%include_file(${platform_errno})
+
+%%public_api()
+
+#endif // LLVM_LIBC_ERRNO_H

libc/lib/CMakeLists.txt

 
 add_entrypoint_library(
   llvmlibc
   DEPENDS
+    # errno.h entrypoints
+    __errno_location
+
     # string.h entrypoints
-    ## C standard library entrypoints
     strcpy
     strcat
 )
 
 add_entrypoint_library(
   llvmlibm
   DEPENDS
     # math.h entrypoints
     round
 )
 
 add_redirector_library(
   llvmlibc_redirectors
   DEPENDS
     round_redirector
 )

libc/spec/linux.td

+def Linux : StandardSpec<"Linux"> {
+  HeaderSpec Errno = HeaderSpec<
+      "errno.h",
+      [
+        Macro<"ENOMEDIUM">,
+        Macro<"ENOTBLK">,
+        Macro<"EMEDIUMTYPE">,
+        Macro<"EBADSLT">,
+        Macro<"ECHRNG">,
+        Macro<"ERFKILL">,
+        Macro<"EUSERS">,
+        Macro<"EBADR">,
+        Macro<"EL3HLT">,
+        Macro<"ENOTUNIQ">,
+        Macro<"EXFULL">,
+        Macro<"EHOSTDOWN">,
+        Macro<"EL3RST">,
+        Macro<"ENOPKG">,
+        Macro<"ENOCSI">,
+        Macro<"EUNATCH">,
+        Macro<"EREMCHG">,
+        Macro<"ETOOMANYREFS">,
+        Macro<"EL2HLT">,
+        Macro<"EBADFD">,
+        Macro<"EREMOTEIO">,
+        Macro<"ENAVAIL">,
+        Macro<"ELIBEXEC">,
+        Macro<"ESHUTDOWN">,
+        Macro<"ENOKEY">,
+        Macro<"ESTRPIPE">,
+        Macro<"EKEYREJECTED">,
+        Macro<"ESRMNT">,
+        Macro<"EKEYREVOKED">,
+        Macro<"EBADE">,
+        Macro<"ELIBBAD">,
+        Macro<"EISNAM">,
+        Macro<"EBFONT">,
+        Macro<"EPFNOSUPPORT">,
+        Macro<"EREMOTE">,
+        Macro<"EDEADLOCK">,
+        Macro<"ENONET">,
+        Macro<"EDOTDOT">,
+        Macro<"EKEYEXPIRED">,
+        Macro<"ELIBSCN">,
+        Macro<"ERESTART">,
+        Macro<"EBADRQC">,
+        Macro<"EUCLEAN">,
+        Macro<"ENOANO">,
+        Macro<"ELIBACC">,
+        Macro<"EHWPOISON">,
+        Macro<"ELIBMAX">,
+        Macro<"ESOCKTNOSUPPORT">,
+        Macro<"ENOTNAM">,
+        Macro<"ELNRNG">,
+        Macro<"EL2NSYNC">,
+        Macro<"EADV">,
+        Macro<"ECOMM">,
+      ],
+      [], // Types
+      []  // Functions
+  >;
+
+  let Headers = [
+    Errno,
+  ];
+}

libc/spec/posix.td

+def POSIX : StandardSpec<"POSIX"> {
+  HeaderSpec Errno = HeaderSpec<
+      "errno.h",
+      [
+        Macro<"E2BIG">,
+        Macro<"EACCES">,
+        Macro<"EADDRINUSE">,
+        Macro<"EADDRNOTAVAIL">,
+        Macro<"EAFNOSUPPORT">,
+        Macro<"EAGAIN">,
+        Macro<"EALREADY">,
+        Macro<"EBADF">,
+        Macro<"EBADMSG">,
+        Macro<"EBUSY">,
+        Macro<"ECANCELED">,
+        Macro<"ECHILD">,
+        Macro<"ECONNABORTED">,
+        Macro<"ECONNREFUSED">,
+        Macro<"ECONNRESET">,
+        Macro<"EDEADLK">,
+        Macro<"EDESTADDRREQ">,
+        Macro<"EDQUOT">,
+        Macro<"EEXIST">,
+        Macro<"EFAULT">,
+        Macro<"EFBIG">,
+        Macro<"EHOSTUNREACH">,
+        Macro<"EIDRM">,
+        Macro<"EINPROGRESS">,
+        Macro<"EINTR">,
+        Macro<"EINVAL">,
+        Macro<"EIO">,
+        Macro<"EISCONN">,
+        Macro<"EISDIR">,
+        Macro<"ELOOP">,
+        Macro<"EMFILE">,
+        Macro<"EMLINK">,
+        Macro<"EMSGSIZE">,
+        Macro<"EMULTIHOP">,
+        Macro<"ENAMETOOLONG">,
+        Macro<"ENETDOWN">,
+        Macro<"ENETRESET">,
+        Macro<"ENETUNREACH">,
+        Macro<"ENFILE">,
+        Macro<"ENOBUFS">,
+        Macro<"ENODATA">,
+        Macro<"ENODEV">,
+        Macro<"ENOENT">,
+        Macro<"ENOEXEC">,
+        Macro<"ENOLCK">,
+        Macro<"ENOLINK">,
+        Macro<"ENOMEM">,
+        Macro<"ENOMSG">,
+        Macro<"ENOPROTOOPT">,
+        Macro<"ENOSPC">,
+        Macro<"ENOSR">,
+        Macro<"ENOSTR">,
+        Macro<"ENOSYS">,
+        Macro<"ENOTCONN">,
+        Macro<"ENOTDIR">,
+        Macro<"ENOTEMPTY">,
+        Macro<"ENOTRECOVERABLE">,
+        Macro<"ENOTSOCK">,
+        Macro<"ENOTSUP">,
+        Macro<"ENOTTY">,
+        Macro<"ENXIO">,
+        Macro<"EOPNOTSUPP">,
+        Macro<"EOVERFLOW">,
+        Macro<"EOWNERDEAD">,
+        Macro<"EPERM">,
+        Macro<"EPIPE">,
+        Macro<"EPROTO">,
+        Macro<"EPROTONOSUPPORT">,
+        Macro<"EPROTOTYPE">,
+        Macro<"EROFS">,
+        Macro<"ESPIPE">,
+        Macro<"ESRCH">,
+        Macro<"ESTALE">,
+        Macro<"ETIME">,
+        Macro<"ETIMEDOUT">,
+        Macro<"ETXTBSY">,
+        Macro<"EWOULDBLOCK">,
+        Macro<"EXDEV">,
+      ],
+      [], // Types
+      []  // Functions
+  >;
+
+  let Headers = [
+    Errno,
+  ];
+}

libc/spec/stdc.td

HeaderSpec StdIO = HeaderSpec<
               [ArgSpec<CharPtr>,
                ArgSpec<SizeTType>,
                ArgSpec<ConstCharRestrictedPtr>,
                ArgSpec<VarArgType>]
           >,
       ]
   >;
 
+  HeaderSpec Errno = HeaderSpec<
+      "errno.h",
+      [
+        Macro<"errno">,
+        Macro<"EDOM">,
+        Macro<"EILSEQ">,
+        Macro<"ERANGE">,
+      ],
+      [], // Types
+      []  // Functions
+  >;
+
   let Headers = [
+    Errno,
     Math,
     String,
     StdIO,
   ];
 }

libc/src/CMakeLists.txt

-add_subdirectory(string)
+add_subdirectory(errno)
 add_subdirectory(math)
+add_subdirectory(string)
 
 add_subdirectory(__support)

libc/src/errno/CMakeLists.txt

+add_entrypoint_object(
+  __errno_location
+  SRCS
+    errno_location.cpp
+  HDRS
+    llvmlibc_errno.h
+)
+
+add_custom_target(libc_errno_unittests)
+
+add_libc_unittest(
+  errno_test
+  SUITE
+    libc_errno_unittests
+  SRCS
+    errno_test.cpp
+  DEPENDS
+    __errno_location
+)

libc/src/errno/errno_location.cpp

+//===----------------- Implementation of __errno_location -----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/errno/llvmlibc_errno.h"
+
+#include "src/__support/common.h"
+
+namespace __llvm_libc {
+
+static thread_local int __errno = 0;
+
+// __errno_location is not really an entry point but we still want it to behave
+// like an entry point because the errno macro resolves to the C symbol
+// "__errno_location".
+int *LLVM_LIBC_ENTRYPOINT(__errno_location)() { return &__errno; }
+
+} // namespace __llvm_libc

libc/src/errno/errno_test.cpp

+//===---------------------- Unittests for errno --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/errno/llvmlibc_errno.h"
+
+#include "gtest/gtest.h"
+
+TEST(ErrnoTest, Basic) {
+  int test_val = 123;
+  llvmlibc_errno = test_val;
+  ASSERT_EQ(test_val, llvmlibc_errno);
+}

libc/src/errno/llvmlibc_errno.h

+//===------------------ Implementation header for errno -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_ERRNO_LLVMLIBC_ERRNO_H
+#define LLVM_LIBC_SRC_ERRNO_LLVMLIBC_ERRNO_H
+
+// Internal code should use this and not use the errno macro from the
+// public header.
+#define llvmlibc_errno (*__llvm_libc::__errno_location())
+
+namespace __llvm_libc {
+
+int *__errno_location();
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_ERRNO_LLVMLIBC_ERRNO_H