Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943863002:
Set using_sanitizer if use_cfi_diag is true. (Closed)
DescriptionSet using_sanitizer if use_cfi_diag is true.
The most important thing that this gives us is the symbolizer data
dependencies in //base (for producing symbolized stack traces on the
swarming bots) but it also allows us to clean up the gn files in a
few places.
R=thakis@chromium.org
BUG=732652
Review-Url: https://codereview.chromium.org/2943863002
Cr-Commit-Position: refs/heads/master@{#480933}
Committed: https://chromium.googlesource.com/chromium/src/+/d13a0f6d7d1fee7ab355ca1b8d3abc2b8acf6978
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add comment #Patch Set 3 : Sanitizers not supported in official builds #
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by pcc@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: This issue passed the CQ dry run.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm.
The CQ bit was checked by pcc@chromium.org
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 thakis@chromium.org
On 2017/06/18 23:09:16, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I didn't stamp this because I wanted to think about how I felt about entangling CFI (generally a prod config) and sanitizers (generally not). I unchecked cq for now so that I can opine on this tomorrow.
On 2017/06/18 23:20:01, Nico wrote: > On 2017/06/18 23:09:16, commit-bot: I haz the power wrote: > > CQ is trying da patch. > > > > Follow status at: > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > I didn't stamp this because I wanted to think about how I felt about entangling > CFI (generally a prod config) and sanitizers (generally not). I unchecked cq for > now so that I can opine on this tomorrow. My take: I think that what using_sanitizer would mean after this change is indeed a little confusing. It basically tells the build system two things: "are we using a runtime library" (and therefore need to disable "-z defs") and "might we need stack traces" (and therefore need to bump the default symbol level, disable frame pointer elimination, ship the symbolizer to swarming, etc). Neither of those things is directly related to sanitizers, but as it happens, right now each of those things are true only if a -fsanitize= flag is passed, and each of those things are true iff the other is true. So we could in principle continue to have a single variable, but I can't see a good name for that variable that would be clearer than using_sanitizer. I'd be happy with either a single variable using_sanitizer with a better-commented definition (unless you can suggest a better name) or a split into two variables called something like using_runtime_library and using_stack_traces (where both would receive the same value right now). The split would mean waiting for a v8 roll, though.
I'd generally be fine with a good comment above using_sanitizer. Looking through `git grep using_sanitizer '*.gn*'` finds a few things that use using_sanitizer for things that seem weird with the new interpretation (e.g. third_party/sqlite/BUILD.gn, chromecast/BUILD.gn) (filed https://bugs.chromium.org/p/chromium/issues/detail?id=734710 for cs.chromium.org's index apparently being incomplete atm) https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:278: cflags += [ "-fno-omit-frame-pointer" ] This still doesn't happen in prod builds, right? Can you maybe add a assert(!is_official_build) here?
On 2017/06/19 19:00:28, pcc1 wrote: > My take: I think that what using_sanitizer would mean after this change is > indeed a little confusing. It basically tells the build system two things: "are > we using a runtime library" (and therefore need to disable "-z defs") and "might > we need stack traces" (and therefore need to bump the default symbol level, > disable frame pointer elimination, ship the symbolizer to swarming, etc). > Neither of those things is directly related to sanitizers, but as it happens, > right now each of those things are true only if a -fsanitize= flag is passed, > and each of those things are true iff the other is true. You lost me here ... aren't there are lots of build configurations where we need stack traces but don't use the sanitizers, like the official builds?
On 2017/06/19 20:38:46, Dirk Pranke wrote: > On 2017/06/19 19:00:28, pcc1 wrote: > > My take: I think that what using_sanitizer would mean after this change is > > indeed a little confusing. It basically tells the build system two things: > "are > > we using a runtime library" (and therefore need to disable "-z defs") and > "might > > we need stack traces" (and therefore need to bump the default symbol level, > > disable frame pointer elimination, ship the symbolizer to swarming, etc). > > Neither of those things is directly related to sanitizers, but as it happens, > > right now each of those things are true only if a -fsanitize= flag is passed, > > and each of those things are true iff the other is true. > > You lost me here ... aren't there are lots of build configurations where we > need stack traces but don't use the sanitizers, like the official builds? Right, of course. I guess I mean something more like "might we need 'live' stack traces from the sanitizer runtime". Which I guess makes the existing name slightly more appropriate.
https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:278: cflags += [ "-fno-omit-frame-pointer" ] That would essentially forbid sanitizers in official builds, right? Is that a good idea?
https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:278: cflags += [ "-fno-omit-frame-pointer" ] On 2017/06/19 22:18:52, pcc1 wrote: > That would essentially forbid sanitizers in official builds, right? Is that a > good idea? I think so? Not sure, but with this change it seems easier to accidentally ship a sanitizer build to prod, and generally we don't want to do this, right? When we ship asan binaries to canary, I'd expect that that would just set is_chrome_branded but not is_official_build, else we grow yet another build config to maintain.
On 2017/06/19 23:50:28, Nico wrote: > https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... > File build/config/sanitizers/BUILD.gn (right): > > https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... > build/config/sanitizers/BUILD.gn:278: cflags += [ "-fno-omit-frame-pointer" ] > On 2017/06/19 22:18:52, pcc1 wrote: > > That would essentially forbid sanitizers in official builds, right? Is that a > > good idea? > > I think so? Not sure, but with this change it seems easier to accidentally ship > a sanitizer build to prod, and generally we don't want to do this, right? > > When we ship asan binaries to canary, I'd expect that that would just set > is_chrome_branded but not is_official_build, else we grow yet another build > config to maintain. I think that sounds correct.
https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2943863002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:278: cflags += [ "-fno-omit-frame-pointer" ] On 2017/06/19 23:50:28, Nico wrote: > On 2017/06/19 22:18:52, pcc1 wrote: > > That would essentially forbid sanitizers in official builds, right? Is that a > > good idea? > > I think so? Not sure, but with this change it seems easier to accidentally ship > a sanitizer build to prod, and generally we don't want to do this, right? > > When we ship asan binaries to canary, I'd expect that that would just set > is_chrome_branded but not is_official_build, else we grow yet another build > config to maintain. Seems reasonable enough. I was imagining that people might want to use a sanitizer to debug something in an official build, but the tradeoff seems to be in favour of hacking the build config locally in those cases.
ptal
The CQ bit was checked by pcc@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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by pcc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2943863002/#ps40001 (title: "Sanitizers not supported in official builds")
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": 1497989932556420,
"parent_rev": "6981621888770efa9113d80998b3dc7fa345842e", "commit_rev":
"d13a0f6d7d1fee7ab355ca1b8d3abc2b8acf6978"}
Message was sent while issue was closed.
Description was changed from ========== Set using_sanitizer if use_cfi_diag is true. The most important thing that this gives us is the symbolizer data dependencies in //base (for producing symbolized stack traces on the swarming bots) but it also allows us to clean up the gn files in a few places. R=thakis@chromium.org BUG=732652 ========== to ========== Set using_sanitizer if use_cfi_diag is true. The most important thing that this gives us is the symbolizer data dependencies in //base (for producing symbolized stack traces on the swarming bots) but it also allows us to clean up the gn files in a few places. R=thakis@chromium.org BUG=732652 Review-Url: https://codereview.chromium.org/2943863002 Cr-Commit-Position: refs/heads/master@{#480933} Committed: https://chromium.googlesource.com/chromium/src/+/d13a0f6d7d1fee7ab355ca1b8d3a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d13a0f6d7d1fee7ab355ca1b8d3a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
