[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc planned changes to this revision. mibintc added a comment. I will prepare another patch responding to joerg's comment: > Quoted Text I had a long discussion with James about this on IRC without reaching a clear consensus. I consider forcing this behavior on all targets to be a major bug. It should be opt-in and opt-in only: (1) The header name is not mandated by any standard. It is not in any namespace generally accepted as implementation-owned. (2) It adds magic behavior that can make debugging more difficult. Partially preprocessed sources for example could be compiled with plain -c before, now they need a different command line. (3) It seems to me that the GNU userland (and maybe Windows) is the exception to a well integrated tool chain. Most other platforms have a single canonical libc, libm and libpthread implementation and can as such directly define all the relevant macros directly in the driver. Given that many of the macros involved are already reflected by the compiler behavior anyway, they can't be decoupled. I.e. the questionable concept of locale-independent wchar_t is already hard-coded in the front end as soon as any long character literals are used. As such, please move the command line additions back into the target-specific files for targets that actually want to get this behavior. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D34158: For standards compatibility, preinclude if the file is available
joerg added a comment. I had a long discussion with James about this on IRC without reaching a clear consensus. I consider forcing this behavior on all targets to be a major bug. It should be opt-in and opt-in only: (1) The header name is not mandated by any standard. It is not in any namespace generally accepted as implementation-owned. (2) It adds magic behavior that can make debugging more difficult. Partially preprocessed sources for example could be compiled with plain -c before, now they need a different command line. (3) It seems to me that the GNU userland (and maybe Windows) is the exception to a well integrated tool chain. Most other platforms have a single canonical libc, libm and libpthread implementation and can as such directly define all the relevant macros directly in the driver. Given that many of the macros involved are already reflected by the compiler behavior anyway, they can't be decoupled. I.e. the questionable concept of locale-independent wchar_t is already hard-coded in the front end as soon as any long character literals are used. As such, please move the command line additions back into the target-specific files for targets that actually want to get this behavior. >Thank you Joerg. Initially I had proposed this only for gnu/Linux. I will >submit another patch like this. As far as I know this is the only toolchain >with this behavior. Please clarify if there are other configurations to cover. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
joerg added a comment. I had a long discussion with James about this on IRC without reaching a clear consensus. I consider forcing this behavior on all targets to be a major bug. It should be opt-in and opt-in only: (1) The header name is not mandated by any standard. It is not in any namespace generally accepted as implementation-owned. (2) It adds magic behavior that can make debugging more difficult. Partially preprocessed sources for example could be compiled with plain -c before, now they need a different command line. (3) It seems to me that the GNU userland (and maybe Windows) is the exception to a well integrated tool chain. Most other platforms have a single canonical libc, libm and libpthread implementation and can as such directly define all the relevant macros directly in the driver. Given that many of the macros involved are already reflected by the compiler behavior anyway, they can't be decoupled. I.e. the questionable concept of locale-independent wchar_t is already hard-coded in the front end as soon as any long character literals are used. As such, please move the command line additions back into the target-specific files for targets that actually want to get this behavior. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc updated this revision to Diff 108999. mibintc added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D34158 Files: include/clang/Driver/CC1Options.td include/clang/Lex/PreprocessorOptions.h lib/Driver/ToolChains/Clang.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/InitPreprocessor.cpp test/Driver/Inputs/stdc-predef/usr/include/stdc-predef.h test/Driver/clang_cpp.c test/Driver/crash-report-header.h test/Driver/crash-report-spaces.c test/Driver/crash-report.c test/Driver/rewrite-legacy-objc.m test/Driver/rewrite-map-in-diagnostics.c test/Driver/rewrite-objc.m test/Driver/stdc-predef-not.c test/Driver/stdc-predef.c test/Index/IBOutletCollection.m test/Index/annotate-macro-args.m test/Index/annotate-tokens-pp.c test/Index/annotate-tokens.c test/Index/c-index-getCursor-test.m test/Index/get-cursor-macro-args.m test/Index/get-cursor.cpp test/Preprocessor/ignore-pragmas.c unittests/Tooling/TestVisitor.h Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -3243,6 +3243,11 @@ KernelOrKext) { CmdArgs.push_back("-ffreestanding"); IsHosted = false; + } else { +// For standards compliance, clang will preinclude +// -ffreestanding suppresses this behavior. +CmdArgs.push_back("-fsystem-include-if-exists"); +CmdArgs.push_back("stdc-predef.h"); } // Forward -f (flag) options which we can pass directly. Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -2503,6 +2503,10 @@ for (const Arg *A : Args.filtered(OPT_chain_include)) Opts.ChainedIncludes.emplace_back(A->getValue()); + // Add the ordered list of -fsystem-include-if-exists. + for (const Arg *A : Args.filtered(OPT_fsystem_include_if_exists)) +Opts.FSystemIncludeIfExists.emplace_back(A->getValue()); + for (const Arg *A : Args.filtered(OPT_remap_file)) { std::pair Split = StringRef(A->getValue()).split(';'); Index: lib/Frontend/InitPreprocessor.cpp === --- lib/Frontend/InitPreprocessor.cpp +++ lib/Frontend/InitPreprocessor.cpp @@ -69,6 +69,15 @@ Builder.append(Twine("#include \"") + File + "\""); } +/// AddImplicitSystemIncludeIfExists - Add an implicit system \#include of the +/// specified file to the predefines buffer: precheck with __has_include. +static void AddImplicitSystemIncludeIfExists(MacroBuilder &Builder, + StringRef File) { + Builder.append(Twine("#if __has_include( <") + File + ">)"); + Builder.append(Twine("#include <") + File + ">"); + Builder.append(Twine("#endif")); +} + static void AddImplicitIncludeMacros(MacroBuilder &Builder, StringRef File) { Builder.append(Twine("#__include_macros \"") + File + "\""); // Marker token to stop the __include_macros fetch loop. @@ -1091,6 +1100,13 @@ // Exit the command line and go back to (2 is LC_LEAVE). if (!PP.getLangOpts().AsmPreprocessor) Builder.append("# 1 \"\" 2"); + + // Process -fsystem-include-if-exists directives. + for (unsigned i = 0, + e = InitOpts.FSystemIncludeIfExists.size(); i != e; ++i) { +const std::string &Path = InitOpts.FSystemIncludeIfExists[i]; +AddImplicitSystemIncludeIfExists(Builder, Path); + } // If -imacros are specified, include them now. These are processed before // any -include directives. Index: unittests/Tooling/TestVisitor.h === --- unittests/Tooling/TestVisitor.h +++ unittests/Tooling/TestVisitor.h @@ -52,6 +52,7 @@ /// \brief Runs the current AST visitor over the given code. bool runOver(StringRef Code, Language L = Lang_CXX) { std::vector Args; +Args.push_back("-ffreestanding"); switch (L) { case Lang_C: Args.push_back("-x"); Index: include/clang/Driver/CC1Options.td === --- include/clang/Driver/CC1Options.td +++ include/clang/Driver/CC1Options.td @@ -738,6 +738,8 @@ HelpText<"Use specified token cache file">; def
RE: [PATCH] D34158: For standards compatibility, preinclude if the file is available
fedor.sergeev added inline comments. Comment at: test/Driver/stdc-predef.c:15 + /* In this test, the file stdc-predef.h is missing from the +installation */ #if _STDC_PREDEF_H + #error "stdc-predef.h should not be included" I would rather see a real check on include file inclusion (say, checking -H output) than a check for a macro. Exact macro guard name is not a public interface at all. You might be lucky with current stdc-predef.h on Linux, but on other platforms it could be named differently. > Thanks Fedor. I am uploading a new patch with an updated version of this > test case. Please see comments in the new patch. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
fedor.sergeev added inline comments. Comment at: test/Driver/stdc-predef.c:15 + /* In this test, the file stdc-predef.h is missing from the installation */ +#if _STDC_PREDEF_H + #error "stdc-predef.h should not be included" I would rather see a real check on include file inclusion (say, checking -H output) than a check for a macro. Exact macro guard name is not a public interface at all. You might be lucky with current stdc-predef.h on Linux, but on other platforms it could be named differently. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc updated this revision to Diff 108669. mibintc added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D34158 Files: include/clang/Driver/CC1Options.td include/clang/Lex/PreprocessorOptions.h lib/Driver/ToolChains/Clang.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/InitPreprocessor.cpp test/Driver/Inputs/stdc-predef/usr/include/stdc-predef.h test/Driver/clang_cpp.c test/Driver/crash-report-header.h test/Driver/crash-report-spaces.c test/Driver/crash-report.c test/Driver/rewrite-legacy-objc.m test/Driver/rewrite-map-in-diagnostics.c test/Driver/rewrite-objc.m test/Driver/stdc-predef-not.c test/Driver/stdc-predef.c test/Index/IBOutletCollection.m test/Index/annotate-macro-args.m test/Index/annotate-tokens-pp.c test/Index/annotate-tokens.c test/Index/c-index-getCursor-test.m test/Index/get-cursor-macro-args.m test/Index/get-cursor.cpp test/Preprocessor/ignore-pragmas.c unittests/Tooling/TestVisitor.h Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -3243,6 +3243,11 @@ KernelOrKext) { CmdArgs.push_back("-ffreestanding"); IsHosted = false; + } else { +// For standards compliance, clang will preinclude +// -ffreestanding suppresses this behavior. +CmdArgs.push_back("-fsystem-include-if-exists"); +CmdArgs.push_back("stdc-predef.h"); } // Forward -f (flag) options which we can pass directly. Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -2502,6 +2502,10 @@ for (const Arg *A : Args.filtered(OPT_chain_include)) Opts.ChainedIncludes.emplace_back(A->getValue()); + // Add the ordered list of -fsystem-include-if-exists. + for (const Arg *A : Args.filtered(OPT_fsystem_include_if_exists)) +Opts.FSystemIncludeIfExists.emplace_back(A->getValue()); + for (const Arg *A : Args.filtered(OPT_remap_file)) { std::pair Split = StringRef(A->getValue()).split(';'); Index: lib/Frontend/InitPreprocessor.cpp === --- lib/Frontend/InitPreprocessor.cpp +++ lib/Frontend/InitPreprocessor.cpp @@ -69,6 +69,15 @@ Builder.append(Twine("#include \"") + File + "\""); } +/// AddImplicitSystemIncludeIfExists - Add an implicit system \#include of the +/// specified file to the predefines buffer: precheck with __has_include. +static void AddImplicitSystemIncludeIfExists(MacroBuilder &Builder, + StringRef File) { + Builder.append(Twine("#if __has_include( <") + File + ">)"); + Builder.append(Twine("#include <") + File + ">"); + Builder.append(Twine("#endif")); +} + static void AddImplicitIncludeMacros(MacroBuilder &Builder, StringRef File) { Builder.append(Twine("#__include_macros \"") + File + "\""); // Marker token to stop the __include_macros fetch loop. @@ -1091,6 +1100,13 @@ // Exit the command line and go back to (2 is LC_LEAVE). if (!PP.getLangOpts().AsmPreprocessor) Builder.append("# 1 \"\" 2"); + + // Process -fsystem-include-if-exists directives. + for (unsigned i = 0, + e = InitOpts.FSystemIncludeIfExists.size(); i != e; ++i) { +const std::string &Path = InitOpts.FSystemIncludeIfExists[i]; +AddImplicitSystemIncludeIfExists(Builder, Path); + } // If -imacros are specified, include them now. These are processed before // any -include directives. Index: unittests/Tooling/TestVisitor.h === --- unittests/Tooling/TestVisitor.h +++ unittests/Tooling/TestVisitor.h @@ -52,6 +52,7 @@ /// \brief Runs the current AST visitor over the given code. bool runOver(StringRef Code, Language L = Lang_CXX) { std::vector Args; +Args.push_back("-ffreestanding"); switch (L) { case Lang_C: Args.push_back("-x"); Index: include/clang/Driver/CC1Options.td === --- include/clang/Driver/CC1Options.td +++ include/clang/Driver/CC1Options.td @@ -735,6 +735,8 @@ HelpText<"Use specified token cache file">; def detailed_preprocessing_record : Flag<["-"], "detailed-preprocessing-record">, HelpText<"include a detailed record of preprocessing actions">; +def fsystem_include_if_exists : Separate<["-"], "fsystem-include-if-exists">, MetaVarName<"">, + HelpText<"Include system file before parsing if file exists">; //===---
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc planned changes to this revision. mibintc added a comment. @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. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
fedor.sergeev added a comment. In https://reviews.llvm.org/D34158#824079, @jyknight wrote: > In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote: > > > Hmm... I tried this patch and now the following worries me: > > > > - it passes -finclude-if-exists stdc-predef.h on all platforms (say, > > including my Solaris platform that has no system stdc-predef.h) > > > Right, but Solaris probably _ought_ to add one as well, to define those > macros. Point taken, started internal discussion with Solaris header folks. > +1 for using a <> include -- that does seem better. This is the only remaining issue that I would like to see fixed here. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D34158: For standards compatibility, preinclude if the file is available
On Fri, Jul 28, 2017 at 02:07:29PM +, Blower, Melanie wrote: > > > fedor.sergeev added a comment. > > Hmm... I tried this patch and now the following worries me: > > - it passes -finclude-if-exists stdc-predef.h on all platforms (say, > including my Solaris platform that has no system stdc-predef.h) > - it searches all the paths, not just "system include" ones > > That essentially disallows user to have stdc-predef.h include in my own > project, since there is a chance that this user header will be accidentally > included by this hidden machinery. > > >> Yes, I recognize this problem. However, I don't know an acceptable way to > >> solve it. Does anyone have a recommendation? I had tried putting angle > >> brackets around the file name string to imply a system-only search but > >> that didn't work: I guess the >angle brackets are taken to be part of the > >> file name. I believe it's intentional that has_include doesn't recognize > >> the angle, I see test cases that have __has_include( "<...") Quoting from > >> the patch: > // For standards compliance, clang will preinclude > // -ffreestanding suppresses this behavior. > CmdArgs.push_back("-finclude-if-exists"); > CmdArgs.push_back(""); // This doesn't work to restrict > the search to system includes > } > > >I could change the argument scanner for __has_include to recognize the angle > >brackets -- would that be acceptable? Alternatively, I could change the > >flag to be "finclude-if-exists" into "fsystem-include-if-exists". Then I > >could create a new preprocessing keyword(is that the right term?) > >__has_system_include and use that instead of __has_include. > > >I tried this test case with -c -E: > >cat test1.c > >#if __has_include( "stdio.h" ) > >#error it has stdio without angle // This is printed > >#else > >#error it does not have stdio without angle > >#endif > > >#if __has_include( "" ) According to: https://clang.llvm.org/docs/LanguageExtensions.html a proper syntax here is without "s: #if __has_include() regards, Fedor. > >#error it has stdio with angle > >#else > >#error it does not have stdio with angle // This is printed > >#endif > > ] cat stdc-predef.h > #error I was not expecting to see that > ] bin/clang hello-world.c > In file included from :2: > ./stdc-predef.h:1:2: error: I was not expecting to see this! > #error I was not expecting to see this! >^ > 1 error generated. > ] > > > Repository: > rL LLVM > > https://reviews.llvm.org/D34158 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D34158: For standards compatibility, preinclude if the file is available
jyknight added a comment. In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote: > Hmm... I tried this patch and now the following worries me: > > - it passes -finclude-if-exists stdc-predef.h on all platforms (say, > including my Solaris platform that has no system stdc-predef.h) Right, but Solaris probably _ought_ to add one as well, to define those macros. > - it searches all the paths, not just "system include" ones > > That essentially disallows user to have stdc-predef.h include in my own > project, since there is a chance that this user header will be accidentally > included by this hidden machinery. IMO, this is a fairly negligible issue, and so we go *shrug* oh well. +1 for using a <> include -- that does seem better. >> If I make a change like this: CmdArgs.push_back(""); the >> program fails to pre-include std-predef.h; so I assume that there is >> something that needs to be fixed with this line: >> Opts.FIncludeIfExists.emplace_back(A->getValue()); If we want those <> in >> there, but no extra " in there, I would need to study which value is >> returned and fiddle around with the characters to make sure the right thing >> gets put into the input stream. This will take me quite a bit more time >> since I know close to diddly at this point. But, note, that will have no effect w.r.t. this issue for most users, since typically people use "cc -Isomepath", which adds 'somepath' to the list which gets searched by both <> and "" includes. Hardly anyone uses -iquote. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
jyknight added a comment. In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote: > Hmm... I tried this patch and now the following worries me: > > - it passes -finclude-if-exists stdc-predef.h on all platforms (say, > including my Solaris platform that has no system stdc-predef.h) Right, but Solaris probably _ought_ to add one as well, to define those macros. > - it searches all the paths, not just "system include" ones > > That essentially disallows user to have stdc-predef.h include in my own > project, since there is a chance that this user header will be accidentally > included by this hidden machinery. IMO, this is a fairly negligible issue, and so we go *shrug* oh well. +1 for using a <> include -- that does seem better. But, note, that will have no effect w.r.t. this issue for most users, since typically people use "cc -Isomepath", which adds 'somepath' to the list which gets searched by both <> and "" includes. Hardly anyone uses -iquote. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D34158: For standards compatibility, preinclude if the file is available
fedor.sergeev added a comment. Hmm... I tried this patch and now the following worries me: - it passes -finclude-if-exists stdc-predef.h on all platforms (say, including my Solaris platform that has no system stdc-predef.h) - it searches all the paths, not just "system include" ones That essentially disallows user to have stdc-predef.h include in my own project, since there is a chance that this user header will be accidentally included by this hidden machinery. >> Yes, I recognize this problem. However, I don't know an acceptable way to >> solve it. Does anyone have a recommendation? I had tried putting angle >> brackets around the file name string to imply a system-only search but that >> didn't work: I guess the >angle brackets are taken to be part of the file >> name. I believe it's intentional that has_include doesn't recognize the >> angle, I see test cases that have __has_include( "<...") Quoting from the >> patch: // For standards compliance, clang will preinclude // -ffreestanding suppresses this behavior. CmdArgs.push_back("-finclude-if-exists"); CmdArgs.push_back(""); // This doesn't work to restrict the search to system includes } >I could change the argument scanner for __has_include to recognize the angle >brackets -- would that be acceptable? Alternatively, I could change the flag >to be "finclude-if-exists" into "fsystem-include-if-exists". Then I could >create a new preprocessing keyword(is that the right term?) >__has_system_include and use that instead of __has_include. >I tried this test case with -c -E: >cat test1.c >#if __has_include( "stdio.h" ) >#error it has stdio without angle // This is printed >#else >#error it does not have stdio without angle >#endif >#if __has_include( "" ) >#error it has stdio with angle >#else >#error it does not have stdio with angle // This is printed >#endif ] cat stdc-predef.h #error I was not expecting to see that ] bin/clang hello-world.c In file included from :2: ./stdc-predef.h:1:2: error: I was not expecting to see this! #error I was not expecting to see this! ^ 1 error generated. ] Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
fedor.sergeev added a comment. Hmm... I tried this patch and now the following worries me: - it passes -finclude-if-exists stdc-predef.h on all platforms (say, including my Solaris platform that has no system stdc-predef.h) - it searches all the paths, not just "system include" ones That essentially disallows user to have stdc-predef.h include in my own project, since there is a chance that this user header will be accidentally included by this hidden machinery. ] cat stdc-predef.h #error I was not expecting to see that ] bin/clang hello-world.c In file included from :2: ./stdc-predef.h:1:2: error: I was not expecting to see this! #error I was not expecting to see this! ^ 1 error generated. ] Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc updated this revision to Diff 108519. mibintc added a comment. 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. The associated patch for test cases in the tools/extra directory is still valid, it doesn't need updating. Repository: rL LLVM https://reviews.llvm.org/D34158 Files: include/clang/Driver/CC1Options.td include/clang/Lex/PreprocessorOptions.h lib/Driver/ToolChains/Clang.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/InitPreprocessor.cpp test/Driver/clang_cpp.c test/Driver/crash-report-header.h test/Driver/crash-report-spaces.c test/Driver/crash-report.c test/Driver/rewrite-legacy-objc.m test/Driver/rewrite-map-in-diagnostics.c test/Driver/rewrite-objc.m test/Index/IBOutletCollection.m test/Index/annotate-macro-args.m test/Index/annotate-tokens-pp.c test/Index/annotate-tokens.c test/Index/c-index-getCursor-test.m test/Index/get-cursor-macro-args.m test/Index/get-cursor.cpp test/Preprocessor/ignore-pragmas.c unittests/Tooling/TestVisitor.h Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -3243,6 +3243,11 @@ KernelOrKext) { CmdArgs.push_back("-ffreestanding"); IsHosted = false; + } else { +// For standards compliance, clang will preinclude +// -ffreestanding suppresses this behavior. +CmdArgs.push_back("-finclude-if-exists"); +CmdArgs.push_back("stdc-predef.h"); } // Forward -f (flag) options which we can pass directly. Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -2502,6 +2502,10 @@ for (const Arg *A : Args.filtered(OPT_chain_include)) Opts.ChainedIncludes.emplace_back(A->getValue()); + // Add the ordered list of -finclude-if-exists. + for (const Arg *A : Args.filtered(OPT_finclude_if_exists)) +Opts.FIncludeIfExists.emplace_back(A->getValue()); + for (const Arg *A : Args.filtered(OPT_remap_file)) { std::pair Split = StringRef(A->getValue()).split(';'); Index: lib/Frontend/InitPreprocessor.cpp === --- lib/Frontend/InitPreprocessor.cpp +++ lib/Frontend/InitPreprocessor.cpp @@ -69,6 +69,12 @@ Builder.append(Twine("#include \"") + File + "\""); } +static void AddImplicitIncludeIfExists(MacroBuilder &Builder, StringRef File) { + Builder.append(Twine("#if __has_include( \"") + File + "\")"); + Builder.append(Twine("#include \"") + File + "\""); + Builder.append(Twine("#endif")); +} + static void AddImplicitIncludeMacros(MacroBuilder &Builder, StringRef File) { Builder.append(Twine("#__include_macros \"") + File + "\""); // Marker token to stop the __include_macros fetch loop. @@ -1091,6 +1097,12 @@ // Exit the command line and go back to (2 is LC_LEAVE). if (!PP.getLangOpts().AsmPreprocessor) Builder.append("# 1 \"\" 2"); + + // Process -finclude-if-exists directives. + for (unsigned i = 0, e = InitOpts.FIncludeIfExists.size(); i != e; ++i) { +const std::string &Path = InitOpts.FIncludeIfExists[i]; +AddImplicitIncludeIfExists(Builder, Path); + } // If -imacros are specified, include them now. These are processed before // any -include directives. Index: unittests/Tooling/TestVisitor.h === --- unittests/Tooling/TestVisitor.h +++ unittests/Tooling/TestVisitor.h @@ -52,6 +52,7 @@ /// \brief Runs the current AST visitor over the given code. bool runOver(StringRef Code, Language L = Lang_CXX) { std::vector Args; +Args.push_back("-ffreestanding"); switch (L) { case Lang_C: Args.push_back("-x"); Index: include/clang/Driver/CC1Options.td === --- include/clang/Driver/CC1Options.td +++ include/clang/Driver/CC1Options.td @@ -735,6 +735,8 @@ HelpText<"Use specified token cache file">; def detailed_preprocessing_record : Flag<["-"], "detailed-preprocessing-record">, HelpText<"include a detailed record of preprocessing actions">; +def finclude_if_exists : Separate<["-"], "finclude-if-exists">, MetaVarName<"">, + HelpText<"Include file before parsing if file exists">; //===--===// // OpenCL Options Index: include/clang/Lex/PreprocessorOptions.h === --- include/clang/Lex/PreprocessorOptions.h +++ include/clang/Lex/PreprocessorOptions.h @@ -60,6 +60,9 @@ /// \brief Headers that will be converted to chained PCHs in memory. std::vector ChainedIncludes;
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc planned changes to this revision. mibintc added a comment. I'm going to rebase the patch to latest. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc added a comment. Ping. Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D34158: For standards compatibility, preinclude if the file is available
@jyknight I've removed the check on nostdinc. AFAIK, I've addressed all the feedback which I've received on the patch, is it OK to commit? --Melanie -Original Message- From: James Y Knight via Phabricator [mailto:revi...@reviews.llvm.org] Sent: Monday, July 10, 2017 11:57 AM To: Blower, Melanie ; zhangsheng...@huawei.com; olivier...@gmail.com; kalinichev.s...@gmail.com; kf...@kde.org; m...@milianw.de; Keane, Erich ; hahnf...@itc.rwth-aachen.de; mgo...@gentoo.org; fedor.serg...@oracle.com; rich...@metafoo.co.uk; renato.go...@linaro.org Cc: jykni...@google.com; ibiryu...@google.com; cfe-commits@lists.llvm.org; kli...@google.com; simon.dar...@imgtec.com; anastasia.stul...@arm.com; arichardson@gmail.com Subject: [PATCH] D34158: For standards compatibility, preinclude if the file is available jyknight added a comment. This version is still disabling upon -nostdinc, which doesn't make sense to me. You didn't remove that, nor respond explaining why you think it does make sense? https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc added a comment. 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 --Melanie Repository: rL LLVM https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc updated this revision to Diff 105904. mibintc edited the summary of this revision. mibintc added a comment. I removed the check on -nostdinc; and made some updates to the test cases Repository: rL LLVM https://reviews.llvm.org/D34158 Files: include/clang/Driver/CC1Options.td include/clang/Lex/PreprocessorOptions.h lib/Driver/ToolChains/Clang.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/InitPreprocessor.cpp test/Driver/Inputs/stdc-predef/bin/.keep test/Driver/Inputs/stdc-predef/usr/i386-unknown-linux/lib/.keep test/Driver/Inputs/stdc-predef/usr/include/stdc-predef.h test/Driver/Inputs/stdc-predef/usr/lib/.keep test/Driver/Inputs/stdc-predef/usr/x86_64-unknown-linux/lib/.keep test/Driver/clang_cpp.c test/Driver/crash report spaces.c test/Driver/crash-report-header.h test/Driver/crash-report.c test/Driver/rewrite-legacy-objc.m test/Driver/rewrite-map-in-diagnostics.c test/Driver/rewrite-objc.m test/Driver/stdc-predef-not.c test/Driver/stdc-predef.c test/Index/IBOutletCollection.m test/Index/annotate-macro-args.m test/Index/annotate-tokens-pp.c test/Index/annotate-tokens.c test/Index/c-index-getCursor-test.m test/Index/get-cursor-macro-args.m test/Index/get-cursor.cpp test/Preprocessor/ignore-pragmas.c unittests/Tooling/TestVisitor.h Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -3175,6 +3175,11 @@ KernelOrKext) { CmdArgs.push_back("-ffreestanding"); IsHosted = false; + } else { +// For standards compliance, clang will preinclude +// -ffreestanding suppresses this behavior. +CmdArgs.push_back("-finclude-if-exists"); +CmdArgs.push_back("stdc-predef.h"); } // Forward -f (flag) options which we can pass directly. Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -2425,6 +2425,10 @@ for (const Arg *A : Args.filtered(OPT_chain_include)) Opts.ChainedIncludes.emplace_back(A->getValue()); + // Add the ordered list of -finclude-if-exists. + for (const Arg *A : Args.filtered(OPT_finclude_if_exists)) +Opts.FIncludeIfExists.emplace_back(A->getValue()); + for (const Arg *A : Args.filtered(OPT_remap_file)) { std::pair Split = StringRef(A->getValue()).split(';'); Index: lib/Frontend/InitPreprocessor.cpp === --- lib/Frontend/InitPreprocessor.cpp +++ lib/Frontend/InitPreprocessor.cpp @@ -69,6 +69,12 @@ Builder.append(Twine("#include \"") + File + "\""); } +static void AddImplicitIncludeIfExists(MacroBuilder &Builder, StringRef File) { + Builder.append(Twine("#if __has_include( \"") + File + "\")"); + Builder.append(Twine("#include \"") + File + "\""); + Builder.append(Twine("#endif")); +} + static void AddImplicitIncludeMacros(MacroBuilder &Builder, StringRef File) { Builder.append(Twine("#__include_macros \"") + File + "\""); // Marker token to stop the __include_macros fetch loop. @@ -1088,6 +1094,12 @@ // Exit the command line and go back to (2 is LC_LEAVE). if (!PP.getLangOpts().AsmPreprocessor) Builder.append("# 1 \"\" 2"); + + // Process -finclude-if-exists directives. + for (unsigned i = 0, e = InitOpts.FIncludeIfExists.size(); i != e; ++i) { +const std::string &Path = InitOpts.FIncludeIfExists[i]; +AddImplicitIncludeIfExists(Builder, Path); + } // If -imacros are specified, include them now. These are processed before // any -include directives. Index: unittests/Tooling/TestVisitor.h === --- unittests/Tooling/TestVisitor.h +++ unittests/Tooling/TestVisitor.h @@ -52,6 +52,7 @@ /// \brief Runs the current AST visitor over the given code. bool runOver(StringRef Code, Language L = Lang_CXX) { std::vector Args; +Args.push_back("-ffreestanding"); switch (L) { case Lang_C: Args.push_back("-std=c99"); break; case Lang_CXX98: Args.push_back("-std=c++98"); break; Index: include/clang/Driver/CC1Options.td === --- include/clang/Driver/CC1Options.td +++ include/clang/Driver/CC1Options.td @@ -729,6 +729,8 @@ HelpText<"Use specified token cache file">; def detailed_preprocessing_record : Flag<["-"], "detailed-preprocessing-record">, HelpText<"include a detailed record of preprocessing actions">; +def finclude_if_exists : Separate<["-"], "finclude-if-exists">, MetaVarName<"">, + HelpText<"Include file before parsing if file exists">; //===--===// // OpenCL Options Index: include/clang/Lex/PreprocessorOptions.h ===
[PATCH] D34158: For standards compatibility, preinclude if the file is available
mibintc added a comment. In https://reviews.llvm.org/D34158#803752, @jyknight wrote: > This version is still disabling upon -nostdinc, which doesn't make sense to > me. > > You didn't remove that, nor respond explaining why you think it does make > sense? You're right, I should remove the check on nostdinc. Sorry I missed that remark from you initially, that's why I didn't reply. I will upload another patch which removes the check on nostdinc. https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
hfinkel added a comment. In https://reviews.llvm.org/D34158#803752, @jyknight wrote: > This version is still disabling upon -nostdinc, which doesn't make sense to > me. I agree. If I pass -nostdinc, so that I then arrange include paths for my own libc headers, I'd then like to pick up the predef header from that library (if available). There's no clearly right answer for whether `__STDC_NO_THREADS__` is defined, for example, regardless of whether we're using -nostdinc. > You didn't remove that, nor respond explaining why you think it does make > sense? https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34158: For standards compatibility, preinclude if the file is available
jyknight added a comment. This version is still disabling upon -nostdinc, which doesn't make sense to me. You didn't remove that, nor respond explaining why you think it does make sense? https://reviews.llvm.org/D34158 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits