Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842063002:
Pass -no-canonical-prefixes to clang by default. (Closed)
|
Created:
3 years, 8 months ago by Dirk Pranke Modified:
3 years, 6 months ago CC:
chromium-reviews, nodir Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass -no-canonical-prefixes to clang by default.
When clang invokes its subtools, by default it passes a default
path to them as an absolute path. We don't really need this to
be absolute, and having goma stores the the command lines that
in its cache entry keys, which means that differing absolute
paths can cause cache misses. So, this CL tells clang to
stop doing that.
R=thakis@chromium.org, shinyak@chromium.org
BUG=706224
Review-Url: https://codereview.chromium.org/2842063002
Cr-Commit-Position: refs/heads/master@{#478859}
Committed: https://chromium.googlesource.com/chromium/src/+/0e8121522085f344b83cc86c514e4bfb7f7c5a22
Patch Set 1 #
Total comments: 7
Patch Set 2 : update w/ review feedback #
Total comments: 1
Patch Set 3 : add !is_nacl #Messages
Total messages: 31 (14 generated)
Let me know if I've bungled this description of what the flag does or why we want it.
https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:440: # Tells the compiler not to pass the default paths to the tools it invokes. No, it tells the compiler to pass the default paths as relative paths instead of as absolute paths: thakis@thakis:~/src/llvm-build-nolibcxx$ diff <(bin/clang -c test.cc -### 2>&1 | tr " " "\n") <(bin/clang -c test.cc -### -no-canonical-prefixes 2>&1 | tr " " "\n") 12c12 < /usr/local/google/home/thakis/src/llvm-build-nolibcxx/bin --- > bin 14c14 < "/usr/local/google/home/thakis/src/llvm-build-nolibcxx/bin/clang-4.0" --- > "bin/clang" 40c40 < "/usr/local/google/home/thakis/src/llvm-build-nolibcxx/lib/clang/5.0.0" --- > "lib/clang/5.0.0" 52c52 < "/usr/local/google/home/thakis/src/llvm-build-nolibcxx/lib/clang/5.0.0/include" --- > "lib/clang/5.0.0/include" https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:445: cflags += [ "-fno-canonical-prefixes" ] It's spelled -no-canonical-prefixes without f I think
Description was changed from ========== Pass -fno-canonical-prefixes to clang by default. When clang invokes its subtools, by default it passes a default path to look for the tools. We don't really need this path, and passing it includes an absolute path in the command lines that goma uses for its cache entry keys. So, this CL tells clang to stop doing that. R=thakis@chromium.org, shinyak@chromium.org BUG=706224 ========== to ========== Pass -fno-canonical-prefixes to clang by default. When clang invokes its subtools, by default it passes a default path to them as an absolute path. We don't really need this to be absolute, and having goma stores the the command lines that in its cache entry keys, which means that differing absolute paths can cause cache misses. So, this CL tells clang to stop doing that. R=thakis@chromium.org, shinyak@chromium.org BUG=706224 ==========
Description was changed from ========== Pass -fno-canonical-prefixes to clang by default. When clang invokes its subtools, by default it passes a default path to them as an absolute path. We don't really need this to be absolute, and having goma stores the the command lines that in its cache entry keys, which means that differing absolute paths can cause cache misses. So, this CL tells clang to stop doing that. R=thakis@chromium.org, shinyak@chromium.org BUG=706224 ========== to ========== Pass -no-canonical-prefixes to clang by default. When clang invokes its subtools, by default it passes a default path to them as an absolute path. We don't really need this to be absolute, and having goma stores the the command lines that in its cache entry keys, which means that differing absolute paths can cause cache misses. So, this CL tells clang to stop doing that. R=thakis@chromium.org, shinyak@chromium.org BUG=706224 ==========
https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:440: # Tells the compiler not to pass the default paths to the tools it invokes. Thanks. Updated. https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:445: cflags += [ "-fno-canonical-prefixes" ] On 2017/04/25 23:21:07, Nico wrote: > It's spelled -no-canonical-prefixes without f I think Done.
lgtm, thanks. https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:443: # clang, but it isn't. Can you add a # TODO(thakis): Figure out if this could be the default behavior. # TODO(thakis): Expose this in clang-cl if not. type of comment here?
And also, https://bugs.chromium.org/p/chromium/issues/detail?id=706224#c68 -- maybe this isn't needed after your other CL? On Tue, Apr 25, 2017 at 7:29 PM, <thakis@chromium.org> wrote: > lgtm, thanks. > > > https://codereview.chromium.org/2842063002/diff/1/build/ > config/compiler/BUILD.gn > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2842063002/diff/1/build/ > config/compiler/BUILD.gn#newcode443 > build/config/compiler/BUILD.gn:443: # clang, but it isn't. > Can you add a > > # TODO(thakis): Figure out if this could be the default behavior. > # TODO(thakis): Expose this in clang-cl if not. > > type of comment here? > > https://codereview.chromium.org/2842063002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:443: # clang, but it isn't. On 2017/04/25 23:29:20, Nico wrote: > Can you add a > > # TODO(thakis): Figure out if this could be the default behavior. > # TODO(thakis): Expose this in clang-cl if not. > > type of comment here? Sure.
lgtm Goma can handle -no-canonical-prefixes since version 126, which has been rolled a few days before. Could you upload a new patch set? We'd like to do dry run with -no-canonical-prefixes. https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2842063002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:445: cflags += [ "-fno-canonical-prefixes" ] On 2017/04/25 23:26:45, Dirk Pranke wrote: > On 2017/04/25 23:21:07, Nico wrote: > > It's spelled -no-canonical-prefixes without f I think > > Done. Yeah, -no-canonical-prefixes is correct. (Could you upload patch set 2?)
On 2017/04/26 00:52:00, shinyak wrote: > Could you upload a new patch set? We'd like to do dry run with > -no-canonical-prefixes. Uploaded. What do you mean by "do a dry run"?
> Uploaded. What do you mean by "do a dry run"? Sorry, I mean "CQ dry run"
The CQ bit was checked by shinyak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
undo lgtm https://codereview.chromium.org/2842063002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2842063002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:447: if (is_clang && !is_win) { It looks pnacl-clang does not recognize -no-canonical-prefixes, we need to have "&& !is_nacl" for now.
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Updated; please take another look?
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm I'm OK if CQ passes. (But I'll check pitfalls abound pnacl-clang. I'm not sure we need to do something for pnacl-clang)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm going to try and land this now, let's see what happens ...
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2842063002/#ps40001 (title: "add !is_nacl")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497309964970040,
"parent_rev": "5c0b1ec78ac4e1052daf2c6ecf5138c604210e33", "commit_rev":
"0e8121522085f344b83cc86c514e4bfb7f7c5a22"}
Message was sent while issue was closed.
Description was changed from ========== Pass -no-canonical-prefixes to clang by default. When clang invokes its subtools, by default it passes a default path to them as an absolute path. We don't really need this to be absolute, and having goma stores the the command lines that in its cache entry keys, which means that differing absolute paths can cause cache misses. So, this CL tells clang to stop doing that. R=thakis@chromium.org, shinyak@chromium.org BUG=706224 ========== to ========== Pass -no-canonical-prefixes to clang by default. When clang invokes its subtools, by default it passes a default path to them as an absolute path. We don't really need this to be absolute, and having goma stores the the command lines that in its cache entry keys, which means that differing absolute paths can cause cache misses. So, this CL tells clang to stop doing that. R=thakis@chromium.org, shinyak@chromium.org BUG=706224 Review-Url: https://codereview.chromium.org/2842063002 Cr-Commit-Position: refs/heads/master@{#478859} Committed: https://chromium.googlesource.com/chromium/src/+/0e8121522085f344b83cc86c514e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0e8121522085f344b83cc86c514e...
Message was sent while issue was closed.
On 2017/06/13 01:49:49, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/0e8121522085f344b83cc86c514e... This commit seems to introduce a regression inside the CrOS sysroot because it can no longer find the right clang++ executable. More details are in https://crbug.com/712797.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2936053003/ by dpranke@chromium.org. The reason for reverting is: This is possibly breaking CrOS builds, see comment #30 and the updates on crbug.com/712797.. |
