New issue
Advanced search Search tips

Issue 636109 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 640254



Sign in to add a comment

clang-cl should print paths that can be copy-pasted into MSVC in its diagnostics

Project Member Reported by scottmg@chromium.org, Aug 9 2016

Issue description

https://msdn.microsoft.com/en-us/library/027c4t2s.aspx

ref: https://cs.chromium.org/chromium/src/build/toolchain/win/BUILD.gn?rcl=0&l=110

e.g.

[414c42a...]d:\src\cr3\src>ninja -C out\Debug chrome
ninja: Entering directory `out\Debug'
[3->1/5 ~1] CXX obj/chrome/browser/browser/safe_browsing_navigation_throttle.obj
FAILED: obj/chrome/browser/browser/safe_browsing_navigation_throttle.obj
../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showIncludes /FC @obj/chrome/browser/browser/safe_browsing_navigation_throttle.obj.rsp /c ../../chrome/browser/renderer_host/safe_browsing_navigation_throttle.cc /Foobj/chrome/browser/browser/safe_browsing_navigation_throttle.obj /Fd"obj/chrome/browser/browser_cc.pdb"
../../chrome/browser/renderer_host/safe_browsing_navigation_throttle.cc(33,1):  error: unknown type name 'ThrottleCheckResult'; did you mean 'SafeBrowsingNavigationThrottle::ThrottleCheckResult'?
ThrottleCheckResult SafeBrowsingNavigationThrottle::WillStartRequest() {
^~~~~~~~~~~~~~~~~~~
SafeBrowsingNavigationThrottle::ThrottleCheckResult
../..\content/public/browser/navigation_throttle.h(19,8):  note: 'SafeBrowsingNavigationThrottle::ThrottleCheckResult' declared here
  enum ThrottleCheckResult {
       ^
../../chrome/browser/renderer_host/safe_browsing_navigation_throttle.cc(36,1):  error: unknown type name 'ThrottleCheckResult'; did you mean 'SafeBrowsingNavigationThrottle::ThrottleCheckResult'?
ThrottleCheckResult SafeBrowsingNavigationThrottle::WillRedirectRequest() {
^~~~~~~~~~~~~~~~~~~
SafeBrowsingNavigationThrottle::ThrottleCheckResult
../..\content/public/browser/navigation_throttle.h(19,8):  note: 'SafeBrowsingNavigationThrottle::ThrottleCheckResult' declared here
  enum ThrottleCheckResult {
       ^


These paths would be "d:\src\cr3\src\chrome/browser/renderer_host/safe_browsing_navigation_throttle.cc" with cl.exe, rather than "../../chrome/browser/renderer_host/safe_browsing_navigation_throttle.cc" as they are with clang-cl.
 
This one is actually intentional (https://bugs.chromium.org/p/chromium/issues/detail?id=504658#c5). Without /FC cl only prints the basename I think, while clang-cl already prints a qualified path, which seems good enough while also keeping the build output fairly computer-independent (no host-specific prefix), which seemed nice.

Do you need full absolute paths for something?
Hmm, nope, cl definitely prints the full path (at least in the chrome build) because that's what I copy/paste to open errors in my editor.

If there was no -C out/Debug, it'd be OK, but the ../.. isn't "right" relative to src where I'm running ninja from.

I dunno. Not urgent anyway. (it'd be nice if the slashes were consistent too...)
I mean I thought cl without /FC only prints the basename. If that's true, you kind of have to pass /FC to cl.exe to make its output at all useful, while clang-cl's relative paths already are kind of useful.
Oh sorry, I see what you mean. OK, maybe it's only me that cares.

... apparently it wasn't me that added it to gyp, so maybe someone else will will be a tiny bit sad https://chromiumcodereview.appspot.com/10537170 . <shrug>

Comment 5 by ajha@chromium.org, Aug 11 2016

Components: Build
This feels like a regression to me. As part of my workflow I depend on being able to copy compiler errors/warnings from the build output, paste them into the VS output window, and then double-click my way through them in order to easily navigate the issues.

It is certainly possible to navigate to the source files in other ways, but definitely more cumbersome. I think other people will this as well.

The printing of slashes instead of back slashes also complicates things (I'm not sure whether that is clang or GN) because it means that paths can't be pasted into the file open dialog without some fixing up.

Comment 7 by h...@chromium.org, Aug 15 2016

Owner: h...@chromium.org
Status: Assigned (was: Unconfirmed)

Comment 8 by thakis@chromium.org, Aug 22 2016

Clicking on stuff sounds like an important use case, we should come up with something to keep that working. (Is it possible to tell msvc "the pwd is out/release"? If so, then that'd make things work, right?)

If we change something here, we should try to come up with something that works on all platforms. I think it's kind of nice that the output is mostly the same everywhere (modulo line/column number formatting).

For slashes / backslashes, what I think happens is that gn uses slashes everywhere, so clang-cl uses slashes for the bits it gets from gn. When it joins to paths, it joins them with a backslash, so we end up with mixed directions. Not sure what the desired end state here should be. We want to keep slashes in gn files. clang-cl probably shouldn't flip slashes to backslashes when reading inputs. clang-cl using backslashes to join paths on windows seems kind of reasonable (but having slashes throughout would make it easier to copy-paste bot output paths on non-windows machines). So every step along the way kind of makes sense, but the final place we end up at kind of doesn't.
I don't know of any way to get msvc to pretend to have a fake pwd. (Or do you mean msvs, i.e. the IDE? ... I don't know how to do that either. :)

I'm a bit uncertain on the rationale for ignoring /FC. It's an option that thing-you've-tried-really-hard-to-match-in-obscure-ways has, and relative paths aren't what it does.

re: slashes: agreed to almost all, but I think clang-cl should output as (all) backslashes when writing error message on Windows to be a "Windows-y" program. I don't care too much, but I guess non-Unix-tool users will, because Windows GUI programs are less likely to accept slashes in place of backslashes. Using slashes in GN was only tolerable on Windows because cl.exe with /FC outputs full paths using backslashes.

Comment 10 by h...@chromium.org, Aug 22 2016

FWIW, I'm planning to look into supporting /FC in clang-cl if it doesn't turn out to be too impractical. I just haven't gotten to that point on my todo list yet.
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be2fa1a7f40f37fb052265b2183b65b3818c6682

commit be2fa1a7f40f37fb052265b2183b65b3818c6682
Author: hans <hans@chromium.org>
Date: Fri Aug 26 23:26:26 2016

Use -fdiagnostics-absolute-paths in Win-Clang builds

BUG= 636109 

Review-Url: https://codereview.chromium.org/2281963002
Cr-Commit-Position: refs/heads/master@{#414856}

[modify] https://crrev.com/be2fa1a7f40f37fb052265b2183b65b3818c6682/build/config/compiler/BUILD.gn

Comment 12 by h...@chromium.org, Aug 26 2016

Blockedon: 640254
Status: Started (was: Assigned)
We need to roll past Clang r279827 for this.
Summary: clang-cl should print paths that can be copy-pasted into MSVC in its diagnostics (was: clang-cl doesn't handle /FC)
retitling to what the actual request is, and what we're doing
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a033f395bf2547cf4764f77cc9c86d08f3e22c23

commit a033f395bf2547cf4764f77cc9c86d08f3e22c23
Author: thakis <thakis@chromium.org>
Date: Wed Aug 31 05:16:14 2016

Roll clang 278861:280106.

* win: Members of base classes now should show up in debugger.
* win: Debugger shouldn't show funny highlights anymore due to
  debug info no longer including column information.  (we still
  force this on if sanitizers are used, mostly for clusterfuzz.
  maybe we want to make this toggleable independent of sanitizers
  at some point)
* win: -Wextern-initializer no longer warns on midl-generated code
* win: clang-cl now accepts /source-encoding:utf-8 and friends
  (utf-8 was the source enconding in clang-cl before already, but
  now we don't warn on an explicit flag requesting this)
* all platforms: Three plugin checks are now on-by-default,
  remove flags for these (see
    https://codereview.chromium.org/2267713003
    https://codereview.chromium.org/2268203002
    https://codereview.chromium.org/2265093002
  )
* win: clang-cl's /Brepro now does what it's supposed to do
* win: clang-cl now emits absolute paths in diagnostics, by
  popular request.

Ran `tools/clang/scripts/upload_revision.py 280106`.

BUG= 640254 , 637456 , 636109 , 636091 , 636099 

Review-Url: https://codereview.chromium.org/2292173002
Cr-Commit-Position: refs/heads/master@{#415563}

[modify] https://crrev.com/a033f395bf2547cf4764f77cc9c86d08f3e22c23/build/config/clang/BUILD.gn
[modify] https://crrev.com/a033f395bf2547cf4764f77cc9c86d08f3e22c23/build/config/compiler/BUILD.gn
[modify] https://crrev.com/a033f395bf2547cf4764f77cc9c86d08f3e22c23/build/config/sanitizers/BUILD.gn
[modify] https://crrev.com/a033f395bf2547cf4764f77cc9c86d08f3e22c23/build/config/win/BUILD.gn
[modify] https://crrev.com/a033f395bf2547cf4764f77cc9c86d08f3e22c23/tools/clang/scripts/update.py

Status: Fixed (was: Started)
https://chromium-review.googlesource.com/c/558871 disabled absolute paths for cl.exe, to my surprise.

Sign in to add a comment