mibintc updated this revision to Diff 105671.
mibintc added a comment.
I updated the patch as James directed, moving the preinclude of stdc-predef.h
into clang.cpp and out of the Linux toolchain entirely. I also needed to
update a couple more tests in tools/extra, so I will need to update that
jyknight added inline comments.
Comment at: lib/Driver/ToolChains/Linux.cpp:755
+!Version.isOlderThan(4, 8, 0)) {
+ // For gcc >= 4.8.x, clang will preinclude
+ // -ffreestanding suppresses this behavior.
jyknight wrote:
> I don't see why it
mibintc updated this revision to Diff 105526.
mibintc added a comment.
I rewrote the patch the way that Richard suggested, adding a cc1 option
"finclude-if-exists" and injecting #if __has_include etc. [OK to use finclude?
include-if-exists preferred?]
I responded to James' remarks and removed
jyknight added inline comments.
Comment at: lib/Driver/ToolChains/Linux.cpp:755
+!Version.isOlderThan(4, 8, 0)) {
+ // For gcc >= 4.8.x, clang will preinclude
+ // -ffreestanding suppresses this behavior.
I don't see why it makes any sense to
On 5 July 2017 at 14:09, Melanie Blower via Phabricator <
revi...@reviews.llvm.org> wrote:
> mibintc added a comment.
>
> Jonas asked about adding a new test to ensure that "-include
> stdc-predef.h" does not get added if the file doesn't exist.
>
> I added a reply to that but I can't see where
mibintc added a comment.
Jonas asked about adding a new test to ensure that "-include stdc-predef.h"
does not get added if the file doesn't exist.
I added a reply to that but I can't see where it went. So I'm writing the reply
again.
The current version of the patch doesn't check for the
mibintc updated this revision to Diff 105329.
mibintc added a comment.
I responded to the review from Jonas Hahnfeld: I moved the entire change into
ToolChains/Linux and removed the modifications to Gnu.h and Gnu.cpp; I modified
the new tests to use -### with FileCheck, and added a tree in
mibintc added a comment.
In https://reviews.llvm.org/D34158#797968, @Hahnfeld wrote:
> In https://reviews.llvm.org/D34158#797170, @mibintc wrote:
>
> > The other test that fails is my own new test! It fails because I don't know
> > how to set it up so the test thinks it has a gcc toolchain with
ilya-biryukov added a comment.
Here's a patch to fix ClangdTests: https://reviews.llvm.org/D34936
We should probably land it before your patch to avoid ClangdTests failures
between those llvm and clang-tools-extra revisions.
Repository:
rL LLVM
https://reviews.llvm.org/D34158
Hahnfeld added a comment.
In https://reviews.llvm.org/D34158#797170, @mibintc wrote:
> The other test that fails is my own new test! It fails because I don't know
> how to set it up so the test thinks it has a gcc toolchain with version >
> 4.8. I tried using gcc-toolchain set to various other
mibintc updated this revision to Diff 104911.
mibintc added a subscriber: ilya-biryukov.
mibintc added a comment.
The previous (n-1) version didn't work: I wanted to iterate through the system
include directories to check for the existence of stdc-predef.h but clang is
using a different kind of
mibintc planned changes to this revision.
mibintc added a comment.
I'm planning to rework this patch again. sorry for the churn.
Repository:
rL LLVM
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Hahnfeld added a subscriber: cfe-commits.
Hahnfeld edited reviewers, added: rsmith, rengolin; removed: cfe-commits.
Hahnfeld added a comment.
Some comments inline. In general you should consider posting an RFC on cfe-dev
because this change will basically affect all compilations on GNU/Linux if
fedor.sergeev added a comment.
> can you recommend someone to review the extra test changes?
I'm rather new to the project and thus rather bad at finding reviewers.
Also generally I do not consider myself qualified enough to be the only
reviewer, though in this particular case I believe I spent
mibintc added a comment.
@fedor.sergeev do you have time to review my (hopefully) final revision? Also
can you recommend someone to review the extra test changes?
Repository:
rL LLVM
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
mibintc updated this revision to Diff 103871.
mibintc added a comment.
Here's a modified revision which checks if stdc-predef.h exists before adding
a -include option to that file. this is only for Linux using gcc 4.8 and
higher. Several tests needed to be fixed because of this change - i
mibintc abandoned this revision.
mibintc added a comment.
I want to submit a modified patch
Repository:
rL LLVM
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mibintc planned changes to this revision.
mibintc added a comment.
I'm submitting another revision which checks for the existence of stdc-predef.h
before inserting the -include option.
Repository:
rL LLVM
https://reviews.llvm.org/D34158
___
mibintc added a comment.
In https://reviews.llvm.org/D34158#782467, @fedor.sergeev wrote:
> LGTM wrt your update to sources.
> And sorry, I'm not that qualified to answer your question on failing tests.
>
> Probing existence of this header would make a sense, yet you are including it
> w/o a
fedor.sergeev added a comment.
LGTM wrt your update to sources.
And sorry, I'm not that qualified to answer your question on failing tests.
Probing existence of this header would make a sense, yet you are including it
w/o a full path, so how are you going to find it for this probe?
mibintc updated this revision to Diff 102605.
mibintc added a comment.
Thanks for your review, Fedor. I moved AddGnuIncludeArgs out of
GCCInstallation, like you suggested, and put it along where the library
includes are added. I also pulled out the change for MipsLinux since I don't
have a
fedor.sergeev added a comment.
> docs are rather clear that this functionality is Linux-only
Oh, well, gcc implementation is rather generic, it relies on gcc's config.gcc
to detect glibc and then it adds a hook that unconditionally provides
stdc-predef.h (TARGET_C_PREINCLUDE).
Currently
fedor.sergeev added inline comments.
Comment at: lib/Driver/ToolChains/Gnu.h:219-220
+void AddGnuIncludeArgs(const llvm::opt::ArgList ,
+ llvm::opt::ArgStringList ) const;
+
To me this does not belong to GCC-Installation-Detector
fedor.sergeev added a comment.
There is no /usr/include/stdc-predef.h on Solaris and I see no sense in adding
this -include for anything except Linux
(as docs are rather clear that this functionality is Linux-only).
Checked gcc on Solaris11/12 both SPARC and X86 - none are doing stdc-predef.h
mibintc updated this revision to Diff 102382.
mibintc added a comment.
forgot to add the new lit test
https://reviews.llvm.org/D34158
Files:
include/clang/Driver/ToolChain.h
lib/Driver/ToolChain.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Gnu.h
mibintc created this revision.
mibintc created this object with visibility "All Users".
Herald added subscribers: fedor.sergeev, arichardson, Anastasia, sdardis,
klimek.
As reported in llvm bugzilla 32377.
Here’s a patch to add preinclude of stdc-predef.h for gcc >= 4.8
The gcc documentation
26 matches
Mail list logo
