[PATCH] D34158: to support gcc 4.8 (and newer) compatibility on Linux, preinclude <stdc-predef.h>

[Jonas Hahnfeld via Phabricator via cfe-commits]

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 the file is present.
Adding Richard (general maintainer) and Renato (ARM Linux) so they are aware.



================
Comment at: include/clang/Driver/ToolChain.h:459-462
+  /// \brief Add arguments to use system-specific GNU includes.
+  virtual void AddGnuIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+                                  llvm::opt::ArgStringList &CC1Args) const;
+
----------------
Why do you need to define this method in the generic `ToolChain`?


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2332-2349
+void Generic_GCC::addGnuIncludeArgs(const ArgList &DriverArgs, 
+                                    ArgStringList &CC1Args) const {
+  const Generic_GCC::GCCVersion &Version = GCCInstallation.getVersion();
+  if (!DriverArgs.hasArg(options::OPT_ffreestanding) &&
+      !DriverArgs.hasArg(clang::driver::options::OPT_nostdinc) &&
+      !Version.isOlderThan(4, 8, 0)) {
+    // If stdc-predef.h exists in the sytem includes, then -include it.
----------------
If this is GNU/Linux only, why not move to the `Linux` toolchain entirely?


================
Comment at: lib/Driver/ToolChains/Gnu.h:328-330
+  void addGnuIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+                         llvm::opt::ArgStringList &CC1Args) const;
+
----------------
This starts with a lower-case character and won't override the method in `ToolChain`. Adding the keyword `override` should reveal this mistake.


================
Comment at: test/Driver/gcc-predef.c:1
+// Test that gcc preincludes stdc-predef.h on Linux
+//
----------------
s/gcc/clang/ ?


================
Comment at: test/Driver/gcc-predef.c:9
+  #else
+    #if !defined(  _STDC_PREDEF_H )
+      #error "stdc-predef.h should be preincluded for GNU/Linux 4.8 and higher"
----------------
The driver will only include `stdc-predef.h` if it can be found in the system. With that, the current version of this test will only run on such Linux system. Maybe add a basic tree in `test/Driver/Inputs` and test that the corresponding header is included?


================
Comment at: test/Driver/gcc-predef.c:14-17
+// Test that gcc preincludes stdc-predef.h on Linux
+//
+// RUN: %clang -c --target=i386-unknown-linux %s -Xclang -verify
+// expected-no-diagnostics
----------------
What's the difference to the test above?


Repository:
  rL LLVM

https://reviews.llvm.org/D34158