Page MenuHomePhabricator

[Preprocessor] Honor absolute paths in diagnostics
Needs ReviewPublic

Authored by aganea on Jun 21 2019, 6:32 AM.

Details

Summary

Previously, -fdiagnostics-absolute-paths did not have an effect in some cases, for example: when using relative include paths, or relative CPP paths, or --show-includes, or -E or displaying notes. We have a peculiar use-case on our end with Fastbuild, where all this was exposed: CPP files are being are preprocessed on one PC, then compiled on another PC (which doesn't have the source-code); then the compiler's stdout is displayed on the first PC.

Diff Detail

Repository
rC Clang

Event Timeline

aganea created this revision.Jun 21 2019, 6:32 AM
aganea added a subscriber: belkiss.Jun 21 2019, 6:35 AM
hans added a comment.Jul 17 2019, 2:59 AM

I will try to take a look, but can you please expand the patch description a little to make it more clear exactly what you're proposing to change? Sorry for all the questions below, I'm just trying to understand exactly what the issue is.

Previously, -fdiagnostics-absolute-paths did not have an effect in some cases, for example: when using relative include paths, or relative CPP paths

Are you saying the diagnostics were not using absolute paths in those cases and this patch fixes that?

, or --show-includes, or -E

Those aren't diagnostics, so that's not surprising.

or displaying notes.

What notes?

We have a peculiar use-case on our end with Fastbuild, where all this was exposed: CPP files are being are preprocessed on one PC, then compiled on another PC (which doesn't have the source-code); then the compiler's stdout is displayed on the first PC.

And what is the final problem? That diagnostics from the compiler's stdout are not absolute because they came from the preprocessed code that doesn't include the absolute paths?

aganea added a comment.EditedJul 17 2019, 9:33 AM

Thanks for getting back Hans!

Are you saying the diagnostics were not using absolute paths in those cases and this patch fixes that?

Yes.

, or --show-includes, or -E

Those aren't diagnostics, so that's not surprising.

What would suggest in that case? Add a new -fpreprocessor-absolute-paths option? Or change the name of -fdiagnostics-absolute-paths for another name that applies to both diagnostics and the preprocessor output?

or displaying notes.

What notes?

That is, the additional output starting with note: issued along error messages: t.cc:4:5: note: candidate function not viable: no known conversion from 'vector<map<[...], float>>' to 'vector<map<[...], double>>' for 1st argument;
Sometimes notes are displaying an extra path, sometimes it's just the note alone.

We have a peculiar use-case on our end with Fastbuild, where all this was exposed: CPP files are being are preprocessed on one PC, then compiled on another PC (which doesn't have the source-code); then the compiler's stdout is displayed on the first PC.

And what is the final problem? That diagnostics from the compiler's stdout are not absolute because they came from the preprocessed code that doesn't include the absolute paths?

Exactly. When clicking on a warning/error, those relative paths prevent Visual Studio from jumping to the right location.

Please let me know if further clarification is needed.

Those aren't diagnostics, so that's not surprising.

What would suggest in that case? Add a new -fpreprocessor-absolute-paths option? Or change the name of -fdiagnostics-absolute-paths for another name that applies to both diagnostics and the preprocessor output?

clang uses the absolute-ness you use on the input. If you pass absolute paths to clang and to -I, then --show-includes etc will print absolute paths:

$ cat test.cc
#include "foo/test.h"
$ out/gn/bin/clang-cl /showIncludes /c test.cc
Note: including file: ./foo/test.h
$ out/gn/bin/clang-cl /showIncludes /c test.cc -I$PWD
Note: including file: /Users/thakis/src/llvm-project/foo/test.h

The problem is that it is not cl.exe's behavior - it always makes paths absolute:

F:\svn\test>cl /c test.cc /showIncludes
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cc
Note: including file: f:\svn\test\foo/test.h

F:\svn\test>c:

C:\Windows\System32>cl /c f:test.cc /showIncludes
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cc
Note: including file: f:\svn\test\foo/test.h

So maybe Driver::IsCLMode() should take precedence over -fdiagnostics-absolute-paths when using /showIncludes?

The behavior between clang-cl and cl is also very different when using -E:

C:\Windows\System32>f:\svn\build\Debug\bin\clang-cl /c f:test.cc /E
clang-cl: error: no such file or directory: 'f:test.cc'
clang-cl: error: no input files

C:\Windows\System32>f:

F:\svn\test>f:\svn\build\Debug\bin\clang-cl /c test.cc /E
# 1 "test.cc"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 361 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "test.cc" 2
# 1 "./foo/test.h" 1
void f();
# 1 "test.cc" 2


F:\svn\test>cl /c test.cc /E
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cc
#line 1 "test.cc"
#line 1 "f:\\svn\\test\\foo/test.h"
void f();
#line 2 "test.cc"

F:\svn\test>cl /c test.cc /E /FC
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cc
#line 1 "f:\\svn\\test\\test.cc"
#line 1 "f:\\svn\\test\\foo\\test.h"
void f();
#line 2 "f:\\svn\\test\\test.cc"

So maybe I should also implement /FC along the way, and make the defaults match between clang-cl and cl? WDYT?

So maybe Driver::IsCLMode() should take precedence over -fdiagnostics-absolute-paths when using /showIncludes?

Definitely not. We want builds to be deterministic; this includes them being independent of the build directory. (This matters for distributed compilation caching.)

So maybe I should also implement /FC along the way, and make the defaults match between clang-cl and cl? WDYT?

We intentionally don't implement /FC for the same reasons, see the discussion on D23816.

If you want absolute paths in the output, you can pass absolute paths to clang-cl and you'll get absolute paths in the output. Does that not work for you?

aganea added a comment.EditedJul 18 2019, 7:00 AM

It totally makes sense, thanks for the explanation Nico! Let's forget about cl compatibility, that wasn't my initial intent.

We always pass relative paths on the cmd-line, for all the reasons you've mentioned. We also pass -fdiagnostics-absolute-paths hoping to fix the Visual Studio jump-to-location issue I've mentioned above. However that doesn't work, because we do:

(locally, on my PC)
  $ clang-cl file.cpp -Isome/relative/path/ -fdiagnostics-absolute-paths /showIncludes -E >file_pre.cpp

(and then, on a remote PC)
  $ clang-cl file_pre.cpp -fdiagnostics-absolute-paths
(the remote stdout is then displayed on my PC)

-fdiagnostics-absolute-paths has no effect in the latter case, because the paths are extracted from the preprocessed file, and paths can't be absolutized anymore on the remote PC because it doesn't have the source code (it is only a worker server).
So we end with relative paths in diagnostics, and Visual Studio can't jump to the location of the diagnostic.

What would you suggest? Use absolute paths along with -fdebug-prefix-map?

It totally makes sense, thanks for the explanation Nico! Let's forget about cl compatibility, that wasn't my initial intent.

We always pass relative paths on the cmd-line, for all the reasons you've mentioned. We also pass -fdiagnostics-absolute-paths hoping to fix the Visual Studio jump-to-location issue I've mentioned above. However that doesn't work, because we do:

(locally, on my PC)
  $ clang-cl file.cpp -Isome/relative/path/ -fdiagnostics-absolute-paths /showIncludes -E >file_pre.cpp

(and then, on a remote PC)
  $ clang-cl file_pre.cpp -fdiagnostics-absolute-paths
(the remote stdout is then displayed on my PC)

-fdiagnostics-absolute-paths has no effect in the latter case, because the paths are extracted from the preprocessed file, and paths can't be absolutized anymore on the remote PC because it doesn't have the source code (it is only a worker server).
So we end with relative paths in diagnostics, and Visual Studio can't jump to the location of the diagnostic.

What would you suggest? Use absolute paths along with -fdebug-prefix-map?

I see, thanks for the details. A few more questions (sorry!):

  • IIRC we used to use fdiagnostics-absolute-paths in Chromium but eventually stopped because someone figured out how to make MSVC's jump-to-diag work even with relative paths (?)

I'm not sure what the Right Fix is here – I can see use cases for -E both with relative and with absolute paths, even if -fdiagnostics-absolute-paths :-/

I think -fdebug-prefix-map doesn't affect -E output as far as I know.

  • Does fastbuild have something like distcc-pump?

Fastbuild works like plain distcc and unfortunately it does not have pump mode.

  • IIRC we used to use fdiagnostics-absolute-paths in Chromium but eventually stopped because someone figured out how to make MSVC's jump-to-diag work even with relative paths (?)

Let me know if you find out what was your solution ;) Otherwise we could absolutize paths in Fastbuild.

While I think we will go the relative paths route - do you think the changes in this patch related to --show-includes and note: make sense? Could -fdiagnostics-absolute-paths affect --show-includes as well? I am just wondering. Otherwise I'll drop the patch.

Maybe we should just implement full /FC mode that affects paths everywhere (diagnostics, __FILE__, etc). Then people who need this more than deterministic, build-directory-independent builds can use that. (But it does break distributed build caching, so it's not that great.)

Not sure.

But -fdiagnostics-absolute-paths should imho not affect /showIncludes output, since that's not a diagnostic.