- User Since
- Nov 14 2016, 12:37 PM (139 w, 6 d)
Mon, Jun 24
Jun 17 2019
This works for me, I redid my patches adding fp-model options to work with this newest changeset, (but I haven't yet uploaded them.)
Jun 3 2019
May 31 2019
Yes I'll do that. Thanks.
I am still waiting for feedback from an actual consumer of the IRBuilder who will be using this new functionality. If someone clang-side could chime in on this ticket I'd very much appreciate it.
I wrote a clang patch that works with your IRBuilder modifications. I also checked with the Intel fortran team and they think this interface will be workable for them too. From Intel perspective it's +1.
May 30 2019
I'll modify this to be a cc1 only option, there doesn't seem to be community interest.
May 20 2019
@rjmccall Can you take a look at this patch or recommend someone who can review it? Many thanks. --Melanie
May 17 2019
@dexonsmith Can you take a look at this patch or recommend someone who can review it? Many thanks. --Melanie
May 10 2019
respond to suggestion from @xbolva00 (thanks). Added a test case where prefix fails to match
May 9 2019
added an inline comment about the use of strncmp
Mar 14 2019
The bug should be fixed at the atomic-load (drop _Atomic qualifier) then this patch won't be needed in StmtExpr
Mar 13 2019
Thanks for test suggestions, will follow up
Feb 13 2019
Jan 9 2019
More info: gcc allows this with -fpermissive, Microsoft gives no message regardless of /permissive-
Apr 17 2018
Mar 21 2018
Hi @mibintc; would you mind confirming that ICC doesn't emit DWARF CC attributes or (if it does) that they don't overlap with the ones in this diff?
Sure i don't mind checking. While icc does support the calling conventions, the compiler is not emitting the dwarf calling convention attributes.
Mar 19 2018
I added some inline comments. You are using the Intel 18.0 compiler on Windows - what version of Visual Studio is in the environment?
Dec 4 2017
added inline replies to Eli and Hubert
I responded to comments from @eli.friedman and @hubert.reinterpretcast : I added FIXME comment regarding the "Q" suffix on the float 128 literals (gcc uses F128 but clang doesn't support this). Hubert pointed out that the floating point macros should only be enabled when float128 is enabled, so I added a check before defining them. I fixed a couple problems that Hubert pointed out in the test case.
Dec 1 2017
I changed the patch to enable _Float128 only as keyword in mode "nocxx" - this is the same mode being used by _Bool. I changed the test from .cpp to .c; I run check-all and saw only the usual suspects fail. What do you think?
Thanks for all your reviews
Nov 30 2017
Nov 22 2017
I also posted this question on IRC
Oct 27 2017
Updated the test diff to correspond to the D34158 patch uploaded today.
The patch to clang is the same as before. The modifications are to the test cases, I verified that all these test cases need modifications. I will also be updating the associated diff for clang/tools/extra, please review that as well. Without the test patches, these are the tests that fail (using check-all) . The failure mode is as described in a note I posted earlier this week, please let me know if you need further details. @jyknight Looking for your review and approval, thanks!
Oct 25 2017
Oct 24 2017
Sep 19 2017
Hey @jyknight I heard from @erichkeane that you may want a couple more changes to this patch. Please let me know if you have some changes to recommend. @joerg thought this was OK for submission. Thanks --Melanie
Sep 12 2017
Updating to latest version. No change other than that.
I heard that @jyknight is still interested in this functionality, updating the patch to latest sources, to do this i needed to make a small change to a couple of driver tests. Other than that it's the same as previous submission.
Sep 7 2017
Another ping. Is it possible to approve this modification, or alternatively, let me know that it won't be approved indefinitely? I understand that it's a controversial change. Thanks!
Aug 17 2017
Aug 14 2017
@jyknight recommended adding the new option to skipArgs in Job.cpp. This revision makes that change and tests that in crash-report.c; look OK?
Aug 11 2017
Need TO FIX: We should be stripping the new arg as well: add "-fsystem-include-if-exists" argument to the list of include things in the skipArgs() function in lib/Driver/Job.cpp
Aug 10 2017
This patch is responding to @jyknight 's concern about preprocessed input. The patch as it stands doesn't have this issue. I added 2 test cases, one using option -x cpp-output, and another for a source file suffixed with .i
Aug 8 2017
Responding to @fedor.sergeev 's comment. This is an updated patch which pulls out the "isValid" check on GCCInstallation. Also I'm putting back the dummy sysroot tree which contains stdc-predef.h and adding a new test run to confirm that the new option fsystem-include-if-exists is actually working.
Aug 7 2017
Updating this patch to latest revision of tools/extra
In the last review, it was deemed less controversial if I move these updates back into the gnu/Linux tool chain. This revision is responding to that feedback. I also simplified the test case. I tested on Linux with check-all and check-clang and found no unexpected failures.
Aug 1 2017
I will prepare another patch responding to joerg's comment:
Jul 31 2017
This patch responds to @fedor.sergeev 's feedback from earlier today. This is a change to the test case test/Driver/stdc-predef.c in the situation that the system does not supply a version of stdc-predef.h. Fedor had suggested an option like -H which traces the include files, but the file stdc-predef.h does not appear in the -H standard include tracing even if it is available on the host system. Instead for this case I preprocess the simple test file, with sysroot pointing to a filesystem missing the file stdc-predef.h, and verify with FileCheck that the string stdc-predef.h doesn't appear in the preprocessed output.
Jul 28 2017
Here's an updated patch which is using angle brackets to do the include, so the search for stdc-predef.h is limited to system directories. Also my previous revision was missing the new test cases since i had gotten a new sandbox but forgot to "svn add". This patch is showing the new test cases.
@erichkeane contacted me offline and pointed out I'm twine-ing with " not <. i'm planning to change the option name from "finclude if exists" to "fsystem include if exists", then i'll quote with < not ". hope to get this updated patch into review later today. let me know if you think i'm off track. thanks for all your careful review. i knew there was something wrong with the include like Fedor pointed out but i couldn't see where i went wrong.
Jul 27 2017
Here is an updated diff which is rebased to current trunk per @fedor.sergeev 's suggestion (thank you). make check-all and check-clang are working for me on Linux with no unexpected failures.
I'm going to rebase the patch to latest.
Jul 26 2017
Jul 18 2017
OK folks, I was off the grid last week but I'm back now, and at my grindstone again.
I haven't received any comments since I updated the patch to remove the checks on "-nostdinc".
Look OK to commit?
Many thanks for all your reviews
Jul 10 2017
This patch corresponds to these modifications to clang: https://reviews.llvm.org/D34158?id=105904
I removed the check on -nostdinc; and made some updates to the test cases
I need to redo the test changes. Will resubmit the diff.
With the latest proposed fix for D34158, a few more test corrections are needed. The correction consists of suppressing the new preprocessor behavior. D34158 preincludes the file <stdc-predef.h> if the file is available.
Jul 9 2017
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 related differential as well.
Jul 6 2017
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 the gcc version check.
Does this look better?
Jul 5 2017
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 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 Inputs/ for the new tests. Jonas please see inline reply concerning whether to--or how to--check for existence of stdc-predef.h
Thanks so much!
Jun 30 2017
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 system include option for /usr and /usr/include so the iterator didn't find any system includes at all, i don't think there is an iterator which will iterate through these special kind of include directories. [Kind of embarrassing that I posted that patch which didn't work at all, sorry about that.]
Jun 28 2017
I'm planning to rework this patch again. sorry for the churn.
Jun 27 2017
@fedor.sergeev do you have time to review my (hopefully) final revision? Also can you recommend someone to review the extra test changes?
Jun 26 2017
Jun 25 2017
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 fixed them by suppressing the preinclude using ffreestanding or nostdinc. Also i need to change about 10 clang/tools/extra/test/clang-tidy tests. Those test changes aren't in this patch. Please tell me: what's the correct procedure for getting those tests reviewed? Thanks Fedor for your reviews!
I want to submit a modified patch
I'm submitting another revision which checks for the existence of stdc-predef.h before inserting the -include option.
Jun 16 2017
Jun 14 2017
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 way to verify that.