@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 review.

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.

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.

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

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.

Address abrachet's comments.

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

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++

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

Address @MaskRay and @alexbrachet's comments.

Address comments.

Abandoning this change now as the patch implementing the ideas showcased here has landed.

In D70197#1754156, @abrachet wrote:

I think this is in a good spot, it's easy to read and modify later as needs change. I don't know much about TableGen, I've tried my best to look through, but it would be preferable if other reviewers would be able to take a look too. Up to your discretion how long you would like to wait to let others chime in. LGTM

Address latest round of comments.

Fix few copy-paste errors pointed out by abrachet; Get rid of Command::Status.

In D70197#1752533, @abrachet wrote:

Not of huge importance, more of curiosity, but could you do what you did with a past revision where you pushed a review with an svg file to your fork so we could see what it looks like on your GitHub?

Address comments.

Improve some comments.

Please wait for saugustine's acceptance.

Use the new header generator to generate string.h.

In D69727#1733518, @gkistanova wrote:

Hello Siva,

How AnnotatedBuilder handles clean arg doesn't seem related to adding libc builders. May I suggest removing this change from your patch and make a separate review item if you want to change that behavior, please?

Use the module libc_vars.

This part of the fix you wanted me to do in https://reviews.llvm.org/D69655

Will put this in on Monday if there no objections.

Do not talk about the SVN repo.

Address comments

My latest diff showcases function attributes and also argument annotations.

Add example of function attrbutes and argument annotations.

Use TableGen types instead of strings.

Add more examples.

In D69020#1723979, @jakehehrlich wrote:

I guess my question is more, whats the evidence that there isn't a way to avoid the two call overhead? The use case makes sense to me but my impression is that __round_redirector could be declared to have extern "C" linkage and then llvm_libc::round can be an alias for it. That gets rid of 1 layer of indirection. Getting rid of the next call seems more gross to me but I think some additional objcopy magic can be done to make __round_redirector an alias for ::round then I think this would all link correctly, right?

In D69020#1721904, @jakehehrlich wrote:

I've missed some emails and the round table at llvm. Could you describe more about why this mechanism for redirecting is used? It seems to me that there should be a way for LLVM_LIBC_ENTRYPOINT(round) to be an alias for ::round rather than requiring two (likely tail) calls here. Could you motivate this mechanism?

This patch illustrates one the desired "features" as proposed in http://llvm.org/docs/Proposals/LLVMLibC.html

Hopefully this should fix the problem you reported here: https://reviews.llvm.org/D67867

In D67867#1698316, @jyknight wrote:

That this is now committed does not change anything w.r.t. needing to respond to outstanding comments.

In D67867#1692484, @MaskRay wrote:

My earlier question is about why we need the namespace __llvm_libc at all. From libc/src/string/strcat/strcat_test.cpp I conclude it is for unit testing in an environment that already has a libc (gtest). This should probably be documented.

Move markdown docs to reStructuredText; Will add conf.py and build targets in a later pass.

In D67867#1691962, @jyknight wrote:

The issue really only exists when you refer to object files across standards layers -- e.g., using an object file that exposes the POSIX symbol "open" from an object file implementing ISO C. If you make sure to always strictly-layer the libc, so that ISO C-implementing object files don't use any POSIX-exporting object files, and so on, you won't need to mark anything weak. For the example of "open", you'd have an internal implementation of open in its own file, only exposing libc-internal symbols. Then fopen (ISO C) can use that safely, without dragging in a definition of the symbol open. Separately, the implementation of open (POSIX) can be defined in its own file, also based on the internal open.)

In D67867#1691626, @lebedev.ri wrote:

Aha. But still, all but few docs migrated back to rst.
I'm not sure it's great to add new ones in the format being-migrated-from.

In D67867#1688779, @jyknight wrote:

In D67867#1688556, @sivachandra wrote:

The objcopy step is required to avoid putting mangled names with the alias attribute. If there is any other way to achieve the same thing, I am open to it.

Ah, I see! I'd suggest using extern "C" instead. There's no need for these be C++-mangled -- you can simply use a name prefix instead. E.g., if you define it as extern "C" __llvm_libc_strcpy(...) {} then it's trivial to make the strcpy alias without objcopy magic.

In D67867#1688297, @jyknight wrote:

Actually, now that I think I understand the existing proposal better, I believe it's broken, as well as confusing. It's getting the same effect as using __attribute__((alias)), except harder to understand. But it's not ok to have a single object file expose both a strong public alias and an internal alias, for any function that's not in the baseline ISO C standard. It would be OK if the aliases were weak, or if they were strong but exposed by a separate .o file. (In any case, I'd like to suggest not using an external objcopy invocation to achieve this.)