Try to shorten system header paths when using -MD depfiles

[Lekensteyn]

Summary

GCC tries to shorten system headers in depfiles using its real path
(resolving components like ".." and following symlinks). Mimic this
feature to ensure that the Ninja build tool detects the correct
dependencies when a symlink changes directory levels, see
https://github.com/ninja-build/ninja/issues/1330

An option to disable this feature is added in case "these changed header
paths may conflict with some compilation environments", see
https://gcc.gnu.org/ml/gcc-patches/2012-09/msg00287.html

Note that the original feature request for GCC
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52974) also included paths
preprocessed output (-E) and diagnostics. That is not implemented now
since I am not sure if it breaks something else.

[Lekensteyn]

I tried to contact Simon (the author of the GCC) patch with a question about the -fno-canonical-system-headers, but his Google address is bouncing.

GCC has long survived with doing this by default, I wonder if this option could just me omitted, enabling the feature by default.

[joerg]

The comments at the very least are misleading. Resolving the path doesn't necessary shorten the string at all, in many cases it will be longer. I'm really not sure if clang should resolve symlinks like this as it can be quite surprising.

[Lekensteyn]

In D37954#873373, @joerg wrote:

Resolving the path doesn't necessary shorten the string at all, in many cases it will be longer.

The description was based on the GCC manual page (" When preprocessing, do not shorten system header paths with canonicalization.")
Maybe something like:
"Do not canonicalize absolute system header paths in depfiles"

I'm really not sure if clang should resolve symlinks like this as it can be quite surprising.

This provides compatibility with GCC and fixes a bug with Ninja. At minimum, this canonicalization should be done when the path contains ".." components.
Can you think of a practical reason why this would cause issues?

[joerg]

ninja is not the only consumer of this. For human inspection, it can be quite surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD. Build systems have different ideas on whether they want absolute resolved paths or not, so it seems like ninja should be able to cope with either format. This is a lossy transformation, so I'm somewhat reluctant to agree with it.

[Lekensteyn]

In D37954#873384, @joerg wrote:

ninja is not the only consumer of this. For human inspection, it can be quite surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD.

No problem, NetBSD disables this option by default:
https://github.com/IIJ-NetBSD/netbsd-src/blob/dd946191000153f9c8a927e5257e726879f48140/share/mk/bsd.sys.mk#L26
This option was added in https://github.com/IIJ-NetBSD/netbsd-src/commit/c7e9228e67fab47a6bbfb548117da93ebb20ff5c
(I am unable to find a (mailing list) discussion about the exact problem though.)

Build systems have different ideas on whether they want absolute resolved paths or not, so it seems like ninja should be able to cope with either format. This is a lossy transformation, so I'm somewhat reluctant to agree with it.

Can you be specific about the build systems you have in mind? Note:

1. GCC has added and enabled this option by default in 2012 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52974). and
2. Ninja is currently slightly broken with Clang whereas it works fine with GCC.
3. To the best of my knowledge, there are no systems that break after this fix.

Looking at the GCC change, it also checks whether the canonicalized path is actually shorter. In that case the documented "shorten header paths" seems to make sense.

I thought it would be a good idea to make Clang compatible with GCC in this case, not sure what else could break.
If it turns out to be bad, it can be removed and/or worked around with the -fno-canonical-system-headers option.

[thakis]

I think for depfile generation, we generally try to be gcc-compatible (I can link to prior changes in this spirit), so I think this seems like a good thing to do to me. gcc only does this for system headers, yes?

Does gcc support the non-negated spelling of this flag (-f-canonical-system-headers) too? If so, we should probably support that too for completeness, even if specifying it explicitly won't change behavior.

[Lekensteyn]

Thanks for the feedback, I have the patch accordingly. Changes in v2:

• Add (no-op) -fcanonical-system-headers option as suggested by @thakis (yes, GCC supports this option, but GCC also has a configure option that can change the default which Clang does not have)
• Check whether path is shorter instead of checking for absolute path (matches the original GCC bug, should address @joerg's concern)
• Update tests to use "%t" instead of "%T" (noticed D35396 after looking for whether "mkdir -p" is allowed) + other minor text/redundancy tweaks

[joerg]

Well, the background for the use of the option in NetBSD is related to inducted differences in reproducable builds. From that perspective, it is even worth to sometimes shorten the dependency and sometimes not.

[Lekensteyn]

Any objection for merging these patches? I rebased today (no changes needed) and it still passes clang-tests.

[Lekensteyn]

ping? I'll push it next week if there is no new feedback.

[rsmith]

In D37954#884029, @Lekensteyn wrote:

Any objection for merging these patches? I rebased today (no changes needed) and it still passes clang-tests.

This is not the way that we do code review in LLVM and Clang. If you don't get a response to a patch, you're expected to keep pinging it (typically once a week) until you do get an answer. This past week in particular has been especially bad as we had the mailing deadline for the C++ committee meeting on Monday and the LLVM developer meeting yesterday and today.

I reverted this in r316195, because it breaks users using -fno-canonical-prefixes to request that we do not do this kind of path canonicalization.

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

967–968

Use hasFlag instead.

Also, defaulting this on will break existing users who use -fno-canonical-prefixes to turn off the (wrong in general) assumption that using realpath is meaningful. Please default to the value of that flag instead. I would guess (but haven't checked) that flag controls this behavior in GCC.

cfe/trunk/lib/Frontend/DependencyFile.cpp

296

This is not an appropriate use of tryGetRealPathName. Its purpose is to get the way the file name was stored in disk (particularly, preserving the original case on a case-sensitive file system), not to resolve symlinks.

FileManager::getCanonicalName would be the right thing to call.

[joerg]

The behavior of sometimes transforming the path and sometimes not is IMO completely unacceptable. I don't care if GCC created that bug first.

[Lekensteyn]

In D37954#901878, @rsmith wrote:

In D37954#884029, @Lekensteyn wrote:

Any objection for merging these patches? I rebased today (no changes needed) and it still passes clang-tests.

This is not the way that we do code review in LLVM and Clang. If you don't get a response to a patch, you're expected to keep pinging it (typically once a week) until you do get an answer.

There was no negative feedback and looking in the git logs also showed some examples without explicit review approval, so I (mistakenly) thought it was acceptable.

This past week in particular has been especially bad as we had the mailing deadline for the C++ committee meeting on Monday and the LLVM developer meeting yesterday and today.

Oh, where could I have learned about this? I am sporadically contributing and not following every day.

I reverted this in r316195, because it breaks users using -fno-canonical-prefixes to request that we do not do this kind of path canonicalization.

I can propose a new patch and also add a test for this case. Thanks for your review and pointers!

In D37954#901884, @joerg wrote:

The behavior of sometimes transforming the path and sometimes not is IMO completely unacceptable. I don't care if GCC created that bug first.

Would it help if a compile-time option is added so you can disable that for OpenBSD? All I am trying to do is to fix a bug in Ninja that is triggered because Clang deviates from GCC here. And Ninja will apparently not be fixed, so that leaves only this option. Do you have an alternative proposal?

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

967–968

Ok, I will apply the suggestions.

This patch does not seem to have any effect on clang, the path still starts with /bin/../lib64/... rather than /usr/lib/...

With GCC: just -no-canonical-prefixes does not have any effect.

-fno-canonical-system-headers -no-canonical-prefixes produces:
/bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio

-fno-canonical-system-headers produces:
/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio

cfe/trunk/lib/Frontend/DependencyFile.cpp

296

OK.

[joerg]

This is not about any operating system, but basic consistent behavior. either do the canonicalisation or not. Doing it sometimes is just bogus. You've effectively implemented -fcanonical-system-headers=sometimes.

[jyknight]

I think the diagnosis on the original issue was incorrect.

It seems to me that it was caused by the prefix being set as "/bin" instead of "/usr/bin", because clang _doesn't_ actually canonicalize its prefix, even when -no-canonical-prefixes isn't specified! All it does, now, is to make the prefix absolute -- without fully canonicalizing. I think that's simply a bug.

If the prefix had been, properly, /usr/bin, then the include path would've been:

/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/iostream

And in that case, it would've worked properly with ninja, without adding the feature in this patch.

Reference clang/tools/driver/driver.cpp:297:

// FIXME: We don't actually canonicalize this, we just make it absolute.
if (CanonicalPrefixes)
  llvm::sys::fs::make_absolute(InstalledPath);

[Lekensteyn]

(FWIW, the suggestion to use FileManager:getCanonicalName did not work because I have no DirectoryEntry. And -no-canonical-prefixes it used very early and stripped from normal options, it seems used for a different purpose. Given the opposition, I think that this patch is a no-go.)

In D37954#902979, @jyknight wrote:

It seems to me that it was caused by the prefix being set as "/bin" instead of "/usr/bin", because clang _doesn't_ actually canonicalize its prefix, even when -no-canonical-prefixes isn't specified! All it does, now, is to make the prefix absolute -- without fully canonicalizing. I think that's simply a bug.

If the prefix had been, properly, /usr/bin, then the include path would've been:

/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/iostream

And in that case, it would've worked properly with ninja, without adding the feature in this patch.

Reference clang/tools/driver/driver.cpp:297:

// FIXME: We don't actually canonicalize this, we just make it absolute.
if (CanonicalPrefixes)
  llvm::sys::fs::make_absolute(InstalledPath);

If this fixes the issue, then it would definitely be a much better approach. If this has changed between Clang 4 -> 5, then it is possible the root cause.

On my system (Arch Linux) /bin is a symlink to usr/bin. Output of clang -v:

clang version 5.0.0 (tags/RELEASE_500/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /bin
Found candidate GCC installation: /bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.0
Found candidate GCC installation: /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0
Found candidate GCC installation: /usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.0
Selected GCC installation: /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64

cfe/trunk/include/clang/Driver/Options.td

 def MP : Flag<["-"], "MP">, Group<M_Group>, Flags<[CC1Option]>,
     HelpText<"Create phony target for each dependency (other than main file)">;
 def MQ : JoinedOrSeparate<["-"], "MQ">, Group<M_Group>, Flags<[CC1Option]>,
     HelpText<"Specify name of main file output to quote in depfile">;
 def MT : JoinedOrSeparate<["-"], "MT">, Group<M_Group>, Flags<[CC1Option]>,
     HelpText<"Specify name of main file output in depfile">;
 def MV : Flag<["-"], "MV">, Group<M_Group>, Flags<[CC1Option]>,
     HelpText<"Use NMake/Jom format for the depfile">;
+def fno_canonical_system_headers : Flag<["-"], "fno-canonical-system-headers">,
+    Group<M_Group>, Flags<[CC1Option]>,
+    HelpText<"Do not shorten system header paths in depfiles">;
+def fcanonical_system_headers : Flag<["-"], "fcanonical-system-headers">,
+    Group<M_Group>;
 def Mach : Flag<["-"], "Mach">, Group<Link_Group>;
 def O0 : Flag<["-"], "O0">, Group<O_Group>, Flags<[CC1Option, HelpHidden]>;
 def O4 : Flag<["-"], "O4">, Group<O_Group>, Flags<[CC1Option, HelpHidden]>;
 def ObjCXX : Flag<["-"], "ObjC++">, Flags<[DriverOption]>,
   HelpText<"Treat source input files as Objective-C++ inputs">;
 def ObjC : Flag<["-"], "ObjC">, Flags<[DriverOption]>,
   HelpText<"Treat source input files as Objective-C inputs">;
 def O : Joined<["-"], "O">, Group<O_Group>, Flags<[CC1Option]>;

cfe/trunk/include/clang/Frontend/DependencyOutputOptions.h

public:
   unsigned IncludeSystemHeaders : 1; ///< Include system header dependencies.
   unsigned ShowHeaderIncludes : 1;   ///< Show header inclusions (-H).
   unsigned UsePhonyTargets : 1;      ///< Include phony targets for each
                                      /// dependency, which can avoid some 'make'
                                      /// problems.
   unsigned AddMissingHeaderDeps : 1; ///< Add missing headers to dependency list
   unsigned PrintShowIncludes : 1; ///< Print cl.exe style /showIncludes info.
   unsigned IncludeModuleFiles : 1; ///< Include module file dependencies.
+  unsigned CanonicalSystemHeaders : 1; ///< Try to output a shorter path for
+                                       /// system header dependencies.
 
   /// The format for the dependency file.
   DependencyOutputFormat OutputFormat;
 
   /// The file to write dependency output to.
   std::string OutputFile;
 
   /// The file to write header include output to. This is orthogonal to

 public:
   DependencyOutputOptions() {
     IncludeSystemHeaders = 0;
     ShowHeaderIncludes = 0;
     UsePhonyTargets = 0;
     AddMissingHeaderDeps = 0;
     PrintShowIncludes = 0;
     IncludeModuleFiles = 0;
+    CanonicalSystemHeaders = 1;
     OutputFormat = DependencyOutputFormat::Make;
   }
 };
 
 }  // end namespace clang
 
 #endif

cfe/trunk/lib/Driver/Job.cpp

IsInclude = llvm::StringSwitch<bool>(Flag)
     .Default(false);
   if (IsInclude)
     return HaveCrashVFS ? false : true;
 
   // The remaining flags are treated as a single argument.
 
   // These flags are all of the form -Flag and have no second argument.
   ShouldSkip = llvm::StringSwitch<bool>(Flag)
-    .Cases("-M", "-MM", "-MG", "-MP", "-MD", true)
-    .Case("-MMD", true)
+    .Cases("-M", "-MM", "-MG", "-MP", "-MD", "-MMD", true)
+    .Cases("-fno-canonical-system-headers", "-fcanonical-system-headers", true)
     .Default(false);
 
   // Match found.
   SkipNum = 1;
   if (ShouldSkip)
     return true;
 
   // These flags are treated as a single argument (e.g., -F<Dir>).

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   Arg *A;
   const bool IsIAMCU = getToolChain().getTriple().isOSIAMCU();
 
   CheckPreprocessingOptions(D, Args);
 
   Args.AddLastArg(CmdArgs, options::OPT_C);
   Args.AddLastArg(CmdArgs, options::OPT_CC);
 
+  if (Arg *A = Args.getLastArg(options::OPT_fno_canonical_system_headers,
+                               options::OPT_fcanonical_system_headers)) {
+    if (A->getOption().matches(options::OPT_fno_canonical_system_headers)) {
+      CmdArgs.push_back("-fno-canonical-system-headers");
+    }
+  }
+
   // Handle dependency file generation.
   if ((A = Args.getLastArg(options::OPT_M, options::OPT_MM)) ||
       (A = Args.getLastArg(options::OPT_MD)) ||
       (A = Args.getLastArg(options::OPT_MMD))) {
     // Determine the output location.
     const char *DepFile;
     if (Arg *MF = Args.getLastArg(options::OPT_MF)) {
       DepFile = MF->getValue();

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

 
 static void ParseDependencyOutputArgs(DependencyOutputOptions &Opts,
                                       ArgList &Args) {
   using namespace options;
   Opts.OutputFile = Args.getLastArgValue(OPT_dependency_file);
   Opts.Targets = Args.getAllArgValues(OPT_MT);
   Opts.IncludeSystemHeaders = Args.hasArg(OPT_sys_header_deps);
   Opts.IncludeModuleFiles = Args.hasArg(OPT_module_file_deps);
+  Opts.CanonicalSystemHeaders = !Args.hasArg(OPT_fno_canonical_system_headers);
   Opts.UsePhonyTargets = Args.hasArg(OPT_MP);
   Opts.ShowHeaderIncludes = Args.hasArg(OPT_H);
   Opts.HeaderIncludeOutputFile = Args.getLastArgValue(OPT_header_include_file);
   Opts.AddMissingHeaderDeps = Args.hasArg(OPT_MG);
   Opts.PrintShowIncludes = Args.hasArg(OPT_show_includes);
   Opts.DOTOutputFile = Args.getLastArgValue(OPT_dependency_dot);
   Opts.ModuleDependencyOutputDir =
       Args.getLastArgValue(OPT_module_dependency_dir);

cfe/trunk/lib/Frontend/DependencyFile.cpp

class DFGImpl : public PPCallbacks {
   const Preprocessor *PP;
   std::string OutputFile;
   std::vector<std::string> Targets;
   bool IncludeSystemHeaders;
   bool PhonyTarget;
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
+  bool CanonicalSystemHeaders;
   DependencyOutputFormat OutputFormat;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
                               SrcMgr::CharacteristicKind FileType);
   void OutputDependencyFile();
 
 public:
   DFGImpl(const Preprocessor *_PP, const DependencyOutputOptions &Opts)
     : PP(_PP), OutputFile(Opts.OutputFile), Targets(Opts.Targets),
       IncludeSystemHeaders(Opts.IncludeSystemHeaders),
       PhonyTarget(Opts.UsePhonyTargets),
       AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
       SeenMissingHeader(false),
       IncludeModuleFiles(Opts.IncludeModuleFiles),
+      CanonicalSystemHeaders(Opts.CanonicalSystemHeaders),
       OutputFormat(Opts.OutputFormat) {
     for (const auto &ExtraDep : Opts.ExtraDeps) {
       AddFilename(ExtraDep);
     }
   }
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
void DFGImpl::FileChanged(SourceLocation Loc,
   const FileEntry *FE =
     SM.getFileEntryForID(SM.getFileID(SM.getExpansionLoc(Loc)));
   if (!FE) return;
 
   StringRef Filename = FE->getName();
   if (!FileMatchesDepCriteria(Filename.data(), FileType))
     return;
 
+  // Try to shorten system header paths like GCC does (unless
+  // -fno-canonical-system-headers is given).
+  if (CanonicalSystemHeaders && isSystem(FileType)) {
+    StringRef RealPath = FE->tryGetRealPathName();
+    if (!RealPath.empty() && RealPath.size() < Filename.size()) {
+      Filename = RealPath;
+    }
+  }
+
   AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
 }
 
 void DFGImpl::InclusionDirective(SourceLocation HashLoc,
                                  const Token &IncludeTok,
                                  StringRef FileName,
                                  bool IsAngled,
                                  CharSourceRange FilenameRange,

cfe/trunk/lib/Tooling/ArgumentsAdjusters.cpp

 
 ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
   return [](const CommandLineArguments &Args, StringRef /*unused*/) {
     CommandLineArguments AdjustedArgs;
     for (size_t i = 0, e = Args.size(); i < e; ++i) {
       StringRef Arg = Args[i];
       // All dependency-file options begin with -M. These include -MM,
       // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-      if (!Arg.startswith("-M"))
+      // The exception is -f[no-]canonical-system-headers.
+      if (!Arg.startswith("-M") && Arg != "-fno-canonical-system-headers" &&
+          Arg != "-fcanonical-system-headers")
         AdjustedArgs.push_back(Args[i]);
 
       if ((Arg == "-MF") || (Arg == "-MT") || (Arg == "-MQ") ||
           (Arg == "-MD") || (Arg == "-MMD")) {
         // Output is specified as -MX foo. Skip the next argument also.
         ++i;
       }
     }

cfe/trunk/test/Preprocessor/dependencies-realpath.c

+// RUN: mkdir -p %t/sub/dir
+// RUN: echo > %t/sub/empty.h
+
+// Test that system header paths are expanded
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem %t/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST1 %s < %t.d
+// TEST1: foo:
+// TEST1: sub{{/|\\}}empty.h
+
+// Test that system header paths are not expanded to a longer form
+//
+// RUN: cd %t && %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem sub/dir/..
+// RUN: FileCheck -check-prefix=TEST2 %s < %t.d
+// TEST2: foo:
+// TEST2: sub/dir/..{{/|\\}}empty.h
+
+// Test that user header paths are not expanded
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %t/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST3 %s < %t.d
+// TEST3: foo:
+// TEST3: sub/dir/..{{/|\\}}empty.h
+
+// Test that system header paths are not expanded with -fno-canonical-system-headers
+// (and also that the -fsystem-system-headers option is accepted)
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %t/sub/dir/.. -fcanonical-system-headers -fno-canonical-system-headers
+// RUN: FileCheck -check-prefix=TEST4 %s < %t.d
+// TEST4: foo:
+// TEST4: sub/dir/..{{/|\\}}empty.h
+
+#include <empty.h>