Page MenuHomePhabricator

[clang] add implicit include for Linux/gnu compatibility
Needs ReviewPublic

Authored by Origami404 on Oct 30 2022, 11:26 AM.

Details

Reviewers
aaron.ballman
erichkeane
jyknight
MaskRay
Group Reviewers
Restricted Project
Restricted Project
Summary

On GNU/Linux, GCC will automatically include stdc-predefs.h, while clang does
not. That is OK for glibc system because glibc includes this file manually. But
with other libc (e.g. musl), which does not manually include this, clang will
fail but GCC can get it compiled.

In 2017, D34158 try to introduce a new flag called fsystem-include-if-exists
to fix this, but it was reverted. Nearly, D106577 points out that macro
__STDC_ISO_10646__ was indeed needed, which is defined in stdc-predefs.h.
After a discussion, the community reaches a consensus, to make this macro
available with a D34158-like method.

In this patch, we port the patch in D34158 into the current clang codebase. The
change is almost the same with D34158, but we change some tests to suit current
codes:

  1. c-index-test now does not accept a -ffreestanding flag, and it should not.

We choose to disable this pre-include action in Objective-C because it will
cause some c-index-test's tests on Objective-C to fail.

  1. Some unit tests about parsing and re-parsing will be affected by this change.

To keep such tests passed, we add -ffreestanding flags to them.

  1. The new tool, Clang-Scan-Deps, will analyze all header files to find

dependencies. After we add an implicit include, its result will have an extra
stdc-predefs.h. Currently, we choose to add -ffreestanding flags to all the
affected tests for it. But maybe making it ignore the default header will be a
better choice.

Signed-off-by: Tao Liang <Origami404@foxmail.com>
Co-authored-by: YingChi Long <me@inclyc.cn>

Link: https://reviews.llvm.org/D34158
Link: https://gcc.gnu.org/gcc-4.8/porting_to.html
Link: https://reviews.llvm.org/D106577

Diff Detail

Event Timeline

Origami404 created this revision.Oct 30 2022, 11:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2022, 11:26 AM
inclyc added a subscriber: inclyc.Oct 30 2022, 11:34 AM
Origami404 published this revision for review.Oct 30 2022, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2022, 11:34 AM

That is OK for glibc system because glibc includes this file manually. But with other libc (e.g. musl), which does not manually include this, clang will fail but GCC can get it compiled.

FWIW currently we cannot build libedit using clang on x86_64-unknown-linux-musl and to fix this issue, they have a workaround here https://patchwork.ozlabs.org/project/buildroot/patch/1452127277-9538-1-git-send-email-sergio.prado@e-labworks.com/ which manually defined __STDC_ISO_10646__

Reproducer:

#include <stdio.h>
// #include <stdc-predef.h>

int main() {
        printf("%d", __STDC_ISO_10646__);
}

GCC accepted this code but clang complained use of undeclared identifier '__STDC_ISO_10646__'

test.c:5:15: error: use of undeclared identifier '__STDC_ISO_10646__'
        printf("%d", __STDC_ISO_10646__);
                     ^
1 error generated.

Note that glibc includes stdc-predefs.h at the source-code level, so this problem cannot be reproduced on the glibc-based systems. (The header stdio.h actually #includes stdc-predefs.h in glibc!)

However if we do not #include stdio.h, different behaviors can be seen on glibc platforms.

int main() {
  return __STDC_ISO_10646__;
}

See https://godbolt.org/z/Gd9Kbn766.

Thanks for the CC. Yep, I had the same adventure with libedit and was very confused for a moment until we dug into it and realised: https://bugs.gentoo.org/870001.

I'm sorry that my patch can not pass the CI test, but I can not reproduce the CI
failure loaclly, including clang-format related problems. On my machine,
ninja check-clang shows:

[0/1] Running the Clang regression tests
llvm-lit: /home/origami/llvm/llvm-project/llvm/utils/lit/lit/llvm/config.py:456: note: using clang: /home/origami/llvm/llvm-project/build-release/bin/clang

Testing Time: 183.65s
  Skipped          :    35
  Unsupported      :  1681
  Passed           : 29839
  Expectedly Failed:    26

And git clang-format HEAD~1 show:

clang-format did not modify any files

I am on Fedora 36, with clang-format 14.0.5 and python 3.10.7. I guess the test
failure maybe lead to incorrect platform setting, so I change the new regression
test in this patch to only run on Linux now by adding -target triple.

I am not familiar with the LLVM pybindings, but I will be more than appreciative
if someone can tell me how to fix the CI failure.

Origami404 added a comment.EditedOct 31 2022, 1:57 AM

After digging more deeply into tests, I found that if we decide to make clang include stdc-predef.h or other things, the behavior of many libtooling-related tools (e.g. clang-tidy, clangd, and python binding) will be affected.

For example, clang-tidy/checkers/cppcoreguidelines/macro-usage.cpp tests improper macro usage. If we #include stdc-predef.h implicitly, it will give warnings to macros that are inside this header. Another example is clang-tidy/checkers/portability/restrict-system-includes-disallow.cpp, it will give errors for including stdc-predef.h.

In my opinion, we may need to spend more time discussing how to make __STD_C_* macros available. The way that D34158 take will affect too many tools using libtooling, which may not be a problem back in 2017.

By the way, I have made a patch to pass tests about clang-tidy and python binding by adding more -ffreestanding flags. But I think it's better to re-think the impact it will make instead of just making tests pass. But I still can not figure out why the git clang-format failed on the CI.

mgorny resigned from this revision.Nov 17 2022, 9:14 AM
mgorny added a reviewer: MaskRay.

I'm afraid this is above my skills. Maybe @MaskRay?

thesamesam added subscribers: mibintc, cor3ntin, Restricted Project, mgorny.

This include-if-exists mechanism seems brittle to me. Can we not make it dependent on the triple, i.e. include the file if we're using a libc implementation that's known to provide (and require) this file?

This include-if-exists mechanism seems brittle to me.

Do you mean the way that we used to test a file and include it (inserting #if __has_include) is brittle or compilation flags like --include-if-exists themselves are?

Can we not make it dependent on the triple, i.e. include the file if we're using a libc implementation that's known to provide (and require) this file?

I am afarid that without depending on triples, we are not able to distinguish what environment we are in or libc we use. Can you explain more about this?

This include-if-exists mechanism seems brittle to me.

Do you mean the way that we used to test a file and include it (inserting #if __has_include) is brittle or compilation flags like --include-if-exists themselves are?

The mechanism itself. For example, we might include the file even if it doesn't belong to the C library implementation that we want to use, just because it's there.

Can we not make it dependent on the triple, i.e. include the file if we're using a libc implementation that's known to provide (and require) this file?

I am afarid that without depending on triples, we are not able to distinguish what environment we are in or libc we use. Can you explain more about this?

I was suggesting to have it depend on the triple, not be independent of it.

Change implements according reviews.

aaron.ballman edited reviewers, added: Restricted Project; removed: mibintc.Nov 30 2022, 7:47 AM

(Removed @mibintc because she retired, added clang-vendors because this can impact other downstreams.)

This is outside my wheelhouse, so I don't have many technical comments to add. I'm hoping @erichkeane and @jyknight can weigh in given how active they were on the earlier thread.

clang/docs/ReleaseNotes.rst
401–402
clang/lib/Driver/ToolChains/Linux.cpp
640
clang/test/Driver/stdc-predef.c
5
12
19
26–27
34–35

What about Fortran?

Improved comments and release notes

Origami404 marked 7 inline comments as done.Nov 30 2022, 9:17 AM
Origami404 added inline comments.
clang/test/Driver/stdc-predef.c
34–35

Only files that using clang to compile will be affected. I have updated my expression here.

I unfortunately am equally not informed enough to help here. The code itself looks fine to me, but I'm unable to make a value judgement on the change itself.

MaskRay added a comment.EditedDec 6 2022, 9:23 AM

In GCC, a *-linux* triple (this applies to musl as well) gets c_target_objs="${c_target_objs} glibc-c.o" from gcc/config.gcc.
"stdc-predef.h" is defined in glibc-c.cc. There is no musl-c.cc.
Even if musl does provide stdc-predef.h and GCC configured on *-linux-musl does include stdc-predef.h, this may be an unexpected use.
Have you tried bring up this issue on https://www.openwall.com/lists/musl/ ?