[PATCH] D34158: For standards compatibility, preinclude if the file is available

[Melanie Blower via Phabricator via cfe-commits]

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

[Blower, Melanie via cfe-commits]

 
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 Sonnenberger via Phabricator via cfe-commits]

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

[Melanie Blower via Phabricator via cfe-commits]

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 , 
+ StringRef File) {
+  Builder.append(Twine("#if __has_include( <") + File + ">)");
+  Builder.append(Twine("#include <") + File + ">");
+  Builder.append(Twine("#endif"));
+}
+
 static void AddImplicitIncludeMacros(MacroBuilder , 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  = 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">;
 

RE: [PATCH] D34158: For standards compatibility, preinclude if the file is available

[Blower, Melanie via cfe-commits]

 
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 via Phabricator via cfe-commits]

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

[Melanie Blower via Phabricator via cfe-commits]

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 , 
+ StringRef File) {
+  Builder.append(Twine("#if __has_include( <") + File + ">)");
+  Builder.append(Twine("#include <") + File + ">");
+  Builder.append(Twine("#endif"));
+}
+
 static void AddImplicitIncludeMacros(MacroBuilder , 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  = 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

[Melanie Blower via Phabricator via cfe-commits]

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 via Phabricator via cfe-commits]

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

[Fedor Sergeev via cfe-commits]

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

[Blower, Melanie via cfe-commits]

 
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

[James Y Knight via Phabricator via cfe-commits]

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

[Blower, Melanie via cfe-commits]

 

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 via Phabricator via cfe-commits]

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

[Melanie Blower via Phabricator via cfe-commits]

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 , StringRef File) {
+  Builder.append(Twine("#if __has_include( \"") + File + "\")");
+  Builder.append(Twine("#include \"") + File + "\"");
+  Builder.append(Twine("#endif"));
+}
+
 static void AddImplicitIncludeMacros(MacroBuilder , 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  = 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 

[PATCH] D34158: For standards compatibility, preinclude if the file is available

[Melanie Blower via Phabricator via cfe-commits]

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

[Melanie Blower via Phabricator via cfe-commits]

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

[Blower, Melanie via cfe-commits]

@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

[Melanie Blower via Phabricator via cfe-commits]

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

[Melanie Blower via Phabricator via cfe-commits]

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 , StringRef File) {
+  Builder.append(Twine("#if __has_include( \"") + File + "\")");
+  Builder.append(Twine("#include \"") + File + "\"");
+  Builder.append(Twine("#endif"));
+}
+
 static void AddImplicitIncludeMacros(MacroBuilder , 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  = 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

[Melanie Blower via Phabricator via cfe-commits]

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

[Hal Finkel via Phabricator via cfe-commits]

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

[James Y Knight via Phabricator via cfe-commits]

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