Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(371)

Issue 201153007: Set the default ASan options for executables built with ASan on Linux. (Closed)

Created:
6 years, 9 months ago by Alexander Potapenko
Modified:
6 years, 8 months ago
Reviewers:
Nico
CC:
chromium-reviews, erikwright+watch_chromium.org
Visibility:
Public.

Description

Set the default ASan options for executables built with ASan on Linux. This is a copy of https://codereview.chromium.org/25687005/ updated for the current trunk with the link_dependency attribute speculatively added to base/base.gyp:sanitizer_options to avoid potential dependency cycles. This CL introduces a module, base/debug/sanitizer_options.cc, which will override the defaults for various dynamic tools (only ASan at this moment). For every executable built with a dynamic tool this module will be linked into that executable, providing weak functions to be called by the tool. The existing declaration of __asan_default_options() in chrome/app/chrome_exe_main_gtk.cc has been moved into sanitizer_options.cc (now every binary built with GOOGLE_CHROME_BUILD=1 will have the same options as google-chrome-asan. The existing declaration of __asan_default_options() in chrome/nacl/nacl_helper_linux.cc has been kept as is, but we had to remove -Wl,-u_sanitizer_options_link_helper to avoid picking sanitizer_options.cc. GYP changes for OSX and iOS will be committed separately. BUG=302040 TBR=thakis@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259561

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Rebased the patch #

Patch Set 4 : removed the verbosity option #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -25 lines) Patch
M base/base.gyp View 1 2 1 chunk +25 lines, -0 lines 0 comments Download
A base/debug/sanitizer_options.cc View 1 2 3 1 chunk +49 lines, -0 lines 0 comments Download
M build/common.gypi View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/app/chrome_exe_main_gtk.cc View 1 2 1 chunk +0 lines, -25 lines 0 comments Download
M components/nacl.gyp View 1 2 1 chunk +6 lines, -0 lines 1 comment Download

Messages

Total messages: 7 (0 generated)
Alexander Potapenko
PTAL
6 years, 9 months ago (2014-03-25 19:47:59 UTC) #1
Alexander Potapenko
Landing with TBR, this had been reviewed already and has been modified only slightly.
6 years, 8 months ago (2014-03-26 09:24:11 UTC) #2
Alexander Potapenko
The CQ bit was checked by glider@chromium.org
6 years, 8 months ago (2014-03-26 09:24:23 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/glider@chromium.org/201153007/60001
6 years, 8 months ago (2014-03-26 09:24:40 UTC) #4
commit-bot: I haz the power
Change committed as 259561
6 years, 8 months ago (2014-03-26 12:52:39 UTC) #5
Nico
lgtm https://codereview.chromium.org/201153007/diff/60001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/201153007/diff/60001/components/nacl.gyp#newcode192 components/nacl.gyp:192: # base/debug/sanitizer_options.cc to avoid th conflict with s/ ...
6 years, 8 months ago (2014-03-26 15:54:03 UTC) #6
tkent
6 years, 8 months ago (2014-03-27 11:40:33 UTC) #7
Message was sent while issue was closed.
A revert of this CL has been created in
https://codereview.chromium.org/212793010/ by tkent@chromium.org.

The reason for reverting is: Broke ChromiumOS ASan build.

http://build.chromium.org/p/chromium.memory/builders/Chromium%20OS%20%28x86%2...
.

Powered by Google App Engine
This is Rietveld 408576698