This is an archive of the discontinued LLVM Phabricator instance.

[clang] improve GCC-compat for C90 __builtin_ functions
Needs ReviewPublic

Authored by nickdesaulniers on Aug 24 2020, 11:50 PM.

Details

Summary

Implements most of the missing C90 __builtin_* functions, and a few
straglers like putchar, fputs, putc, fputc, and free.

Format strings for __builtin_* were copied (and f -> F), but the
signatures for the three above should be more carefully checked.

More importantly, start adding missing tests for
builtin_*/has_builtin, and document more builtins.

Reported-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 11:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers requested review of this revision.Aug 24 2020, 11:50 PM
  • add more docs
nickdesaulniers retitled this revision from [clang] consistently use getLangOpts() to [clang] implement+test remaining C90 __builtin_ functions.Aug 24 2020, 11:58 PM
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers added reviewers: efriedma, rsmith.
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers added subscribers: nathanchance, void.

bumping for review. Are there additional reviewers we could add to share the burden?

LGTM

I've not checked all the types are correct (someone should double-check that prior to commit), but it looks like GCC provides these __builtin_* functions, so we should too. The addition of the non-__builtin_ versions should improve our diagnostics but otherwise have no effect.

clang/lib/Lex/PPMacroExpansion.cpp
348 ↗(On Diff #287573)

The changes to this file appear to be independent of the rest of the patch. Please go ahead and land this cleanup separately.

clang/test/CXX/over/over.oper/over.literal/p7.cpp
10 ↗(On Diff #287573)

Doesn't this also need to be extern "C" to be recognized as the builtin?

aaron.ballman added inline comments.Sep 1 2020, 3:00 PM
clang/include/clang/Basic/Builtins.def
488

Should we be adding atexit() as well?

514

Should we also add a builtin for putc() (I know that's often a macro, so I'm not certain if it applies)?

rsmith added inline comments.Sep 1 2020, 3:05 PM
clang/include/clang/Basic/Builtins.def
488

GCC doesn't have a __builtin_atexit, so we'd need some reason to invent one.

514

Yes, GCC has a __builtin_putc, so it'd make sense for us to support that too.

aaron.ballman added inline comments.Sep 1 2020, 3:06 PM
clang/include/clang/Basic/Builtins.def
488

Ah, I didn't realize this was for GCC compatibility. Thanks!

nickdesaulniers marked an inline comment as done.
  • rebase on master, precommitted getLangOpts() change, dropped p7.cpp test change
clang/include/clang/Basic/Builtins.def
514

Curious, putc isn't documented at https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html, which is what I was using. It looks like putc was a part of ANSI C, so I'm not sure what else might be missing from my implementation. Let me see if I can find a more complete list of C90 functions to verify.

clang/lib/Lex/PPMacroExpansion.cpp
348 ↗(On Diff #287573)
clang/test/CXX/over/over.oper/over.literal/p7.cpp
10 ↗(On Diff #287573)

Interesting, if I change this back to void, the test passes for me. I'm not sure why it ever failed then. Will drop this hunk from the diff.

nickdesaulniers marked 3 inline comments as done.Sep 1 2020, 4:24 PM
nickdesaulniers added inline comments.Sep 1 2020, 4:26 PM
clang/include/clang/Basic/Builtins.def
514

(retrieves copy of The Standard C Library from P. J. Plauger)

nickdesaulniers added inline comments.Sep 1 2020, 4:46 PM
clang/include/clang/Basic/Builtins.def
514

Looks like I'm missing:

From stdlib.h:

  • div
  • ldiv
  • bsearch
  • qsort
  • rand
  • srand
  • atof
  • atoi
  • atol
  • mblen
  • mbstowcs
  • mbtowc
  • wcstombs
  • wctomb
  • abort
  • atexit
  • getenv
  • system

From string.h:

  • strcoll

From stdio.h:

  • remove
  • rename
  • tmpfile
  • fclose
  • fflush
  • freopen
  • setvbuf
  • scanfwrite
  • fgetc
  • fgets
  • fputc
  • getc
  • getchar
  • gets
  • putc
  • ungetc
  • fgetpos
  • fseek
  • fsetpos
  • ftell
  • rewind
  • clearerr
  • feof
  • ferror
  • perror

We look good on ctype.h and stdard.h. Shall I very those against GCC and implement what's missing here?

nickdesaulniers added inline comments.Sep 2 2020, 1:35 PM
clang/include/clang/Basic/Builtins.def
514

(abort should not have been in the above list; it's in GCC's docs and we implement the builtin for it)

From the above list, GCC has builtins for:

  • fputc
  • putc

We already support `abort. Let me:

  1. file a docs bug against GCC for those 2 undocumented builtins.
  2. add implementations of them to this change.
  3. add a test case for pr/47387 just to be safe.
  • add putc and fputc builtins
nickdesaulniers marked 2 inline comments as done.Sep 2 2020, 2:42 PM
nickdesaulniers added inline comments.
clang/include/clang/Basic/Builtins.def
514
  1. Filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96907.
  2. done
  3. as long as clang adds the no-builtin-puts attribute, then pr/47387 is fixed. It may not be interesting to add a test for properly emitting the builtin, since clang/test/CodeGen/libcalls-fno-builtin.c already covers that.
nickdesaulniers planned changes to this revision.Sep 2 2020, 2:43 PM
nickdesaulniers added inline comments.
clang/docs/LanguageExtensions.rst
2439

Let me triple check the docs look good.

nickdesaulniers edited the summary of this revision. (Show Details)Sep 2 2020, 2:59 PM
nickdesaulniers retitled this revision from [clang] implement+test remaining C90 __builtin_ functions to [clang] improve GCC-compat for C90 __builtin_ functions.Sep 2 2020, 3:02 PM
nickdesaulniers edited the summary of this revision. (Show Details)
  • improve docs

Ok, this is ready for review. I plan to review the newer C standards than ISO C90 in follow up patches, so I'll likely be touching these file more. Figuring out a more maintainable sort order in particular is an itch I'd like to scratch.

This looks reasonable to me.

clang/docs/LanguageExtensions.rst
2370

Can you mention the deprecation issue here?

rsmith added inline comments.Sep 3 2020, 12:46 PM
clang/docs/LanguageExtensions.rst
2370–2398

Do we really provide constant evaluation support for all of these functions?

2381

Do we really provide __builtin_strerror? I don't see it below.

2425–2435

You've moved this from the section where we describe the constant evaluation rules for these functions to a section where we describe the constant evaluation rules for string functions; those rules are quite different.

2427–2428

We should say how these "builtin forms" are named and what they're for: that we provide a __builtin_-prefixed version of each of these functions that has the same signature and semantics as the function from the C standard library but doesn't require a header to be included.

It seems to me that adding new __builtin_* functions for GCC compatibility and adding new LIBBUILTINs are unrelated changes, and it might be clearer to split them up into two commits.

clang/docs/LanguageExtensions.rst
2370

LLVM will generate calls to bcmp, see commit 8e16d73346f8 ("[SelectionDAG] Allow the user to specify a memeq function.").

I don't think we should be doing that, if we're going to mention explicitly in our docs that bcmp is deprecated. It feels like a double standard to dissuade its use, but then go and generate calls to it that weren't written explicitly.

2370–2398

Is that what is meant by having "buitins" for these functions? (I'm not sure whether you're arguing that these should be listed somewhere else, or that the current patch is not enough to claim support for __has_builtin for these functions).

2381

We have the LIBBUILTIN rule for it, but not the corresponding BUILTIN rule. Should I add it?