Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654623002:
Enable use-after-scope check in ASAN configs. (Closed)
DescriptionEnable use-after-scope check in ASAN configs.
This is a second attempt to land this change.
Previous attempts failed on some ChromeOS bots
using old version of Clang, on Clang-CL Win bots
and also there were a couple of webkit_tests failed
due to a real use-after-scope issue.
The use-after-scope issue is now fixed by
https://codereview.chromium.org/2649903005/, Windows and
ChromeOS are temporarily blacklisted.
BUG=681136, 683459, 683966, 683445
Review-Url: https://codereview.chromium.org/2654623002
Cr-Commit-Position: refs/heads/master@{#445747}
Committed: https://chromium.googlesource.com/chromium/src/+/a9461af221d3d56769f21b6755a8afbef00d6d7d
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix is_clang and is_win #Messages
Total messages: 28 (16 generated)
The CQ bit was checked by krasin@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
krasin@chromium.org changed reviewers: + thakis@chromium.org
The CQ bit was checked by krasin@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...
https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: if (is_clang && !is_mac && !is_chromeos && !is_win) { this is in is_posix, you don't need !is_win here. also, doesn't is_asan imply is_clang?
https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: if (is_clang && !is_mac && !is_chromeos && !is_win) { On 2017/01/23 22:25:20, Nico wrote: > this is in is_posix, you don't need !is_win here. well, apparently, clang-cl is considered to be posix. See https://crbug.com/683966 > > also, doesn't is_asan imply is_clang? Well, I also thought that is_asan implies the single Clang version, which proved to be not the case for Chrome OS, see https://crbug.com/683445. Now, I don't want to make strong assumptions. I need clang, I have put is_clang.
https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: if (is_clang && !is_mac && !is_chromeos && !is_win) { On 2017/01/23 22:31:51, krasin1 wrote: > On 2017/01/23 22:25:20, Nico wrote: > > this is in is_posix, you don't need !is_win here. > well, apparently, clang-cl is considered to be posix. See > https://crbug.com/683966 That's from asan_flags below, not from default_sanitizer_ld_flags, right? > > > > > also, doesn't is_asan imply is_clang? > Well, I also thought that is_asan implies the single Clang version, which proved > to be not the case for Chrome OS, see https://crbug.com/683445. Now, I don't > want to make strong assumptions. I need clang, I have put is_clang. Hm, I'd prefer if we pretend that we know what's going on even if it means another round of reverts. That seems better than adding lots of just-in-case code all over the place.
https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2654623002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: if (is_clang && !is_mac && !is_chromeos && !is_win) { On 2017/01/23 22:35:02, Nico wrote: > On 2017/01/23 22:31:51, krasin1 wrote: > > On 2017/01/23 22:25:20, Nico wrote: > > > this is in is_posix, you don't need !is_win here. > > well, apparently, clang-cl is considered to be posix. See > > https://crbug.com/683966 > > That's from asan_flags below, not from default_sanitizer_ld_flags, right? ah, right. > > > > > > > > > also, doesn't is_asan imply is_clang? > > Well, I also thought that is_asan implies the single Clang version, which > proved > > to be not the case for Chrome OS, see https://crbug.com/683445. Now, I don't > > want to make strong assumptions. I need clang, I have put is_clang. > > Hm, I'd prefer if we pretend that we know what's going on even if it means > another round of reverts. That seems better than adding lots of just-in-case > code all over the place. that's a valid point. I have removed is_clang.
The CQ bit was checked by krasin@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...
lgt
lgtm
On 2017/01/23 22:58:40, Nico wrote: > lgtm Thank you, Nico. I will land this one after https://codereview.chromium.org/2649903005/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krasin@chromium.org
The CQ bit was unchecked by krasin@chromium.org
The CQ bit was checked by krasin@chromium.org
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": 20001, "attempt_start_ts": 1485278001225940,
"parent_rev": "1761cd38fa199b8204a4e17a20599b17bd4b3f0a", "commit_rev":
"a9461af221d3d56769f21b6755a8afbef00d6d7d"}
Message was sent while issue was closed.
Description was changed from ========== Enable use-after-scope check in ASAN configs. This is a second attempt to land this change. Previous attempts failed on some ChromeOS bots using old version of Clang, on Clang-CL Win bots and also there were a couple of webkit_tests failed due to a real use-after-scope issue. The use-after-scope issue is now fixed by https://codereview.chromium.org/2649903005/, Windows and ChromeOS are temporarily blacklisted. BUG=681136,683459,683966,683445 ========== to ========== Enable use-after-scope check in ASAN configs. This is a second attempt to land this change. Previous attempts failed on some ChromeOS bots using old version of Clang, on Clang-CL Win bots and also there were a couple of webkit_tests failed due to a real use-after-scope issue. The use-after-scope issue is now fixed by https://codereview.chromium.org/2649903005/, Windows and ChromeOS are temporarily blacklisted. BUG=681136,683459,683966,683445 Review-Url: https://codereview.chromium.org/2654623002 Cr-Commit-Position: refs/heads/master@{#445747} Committed: https://chromium.googlesource.com/chromium/src/+/a9461af221d3d56769f21b6755a8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a9461af221d3d56769f21b6755a8...
Message was sent while issue was closed.
On 2017/01/24 17:18:42, commit-bot: I haz the power wrote: > Committed patchset #2 (id:20001) as > https://chromium.googlesource.com/chromium/src/+/a9461af221d3d56769f21b6755a8... on ChromeOS compiler rolled today but there is an issue with Goma. We probably need to way for the Goma issue to clear up before we can enable on ChromeOS.
Message was sent while issue was closed.
On 2017/01/24 21:43:29, llozano wrote: > On 2017/01/24 17:18:42, commit-bot: I haz the power wrote: > > Committed patchset #2 (id:20001) as > > > https://chromium.googlesource.com/chromium/src/+/a9461af221d3d56769f21b6755a8... > > on ChromeOS compiler rolled today but there is an issue with Goma. We probably > need to way for the Goma issue to clear up before we can enable on ChromeOS. Thank you for the update! For the record, this CL does not enable it on ChromeOS, so everything should be fine. |
