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

[Blower, Melanie via 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.
>> Thank you

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`?
>>You're right. I don't need this. I was imitating a similar change I saw to add include directories for a different toolchain. That's why I started out this way. Thank you.

================
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?
>>Yes I will do that, good idea.

================
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.
>>Thanks

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

>>OK
================
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?
>>I'm not sure what you're getting at "will only run on such a Linux system". Are you worried about the case where there's a linux system with gcc 4.8 and there's no such header? In that case this test would fail but I would think that's an outlier/oddball case. When I run this test case on my system, with that version and the file exists, this test did pass. I will study the tests in Driver/Inputs and see about creating a test there.

================
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?
>>Whoops, I don't know how that happened. I will fix it. Thanks for your review, much appreciated.

Repository:
  rL LLVM

https://reviews.llvm.org/D34158