Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1464293002:
If Oilpan is enabled, warn of raw heap pointer fields by default. (Closed)
DescriptionIf Oilpan is enabled, warn of raw heap pointer fields by default.
It is unsafe to keep fields with raw pointers into the Oilpan heap, as
such untraced references risk going stale. With potentially undesirable
consequences.
Now that we've addressed and handled all such untraced references as part
of Blink's transition to Oilpan, it is time to enable the clang GC plugin
warning for such raw pointer uses.
It shouldn't represent a major imposition to developers to handle such
raw pointer uses correctly, but for now we will only emit a warning and
not an error.
R=haraken
BUG=515524
Committed: https://crrev.com/3a192c385e0df065569291963b0aa8a1c992700f
Cr-Commit-Position: refs/heads/master@{#361656}
Patch Set 1 #
Messages
Total messages: 24 (9 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look.
LGTM.
Description was changed from ========== If Oilpan is enabled, warn of raw heap pointer fields by default. It is unsafe to keep fields with raw pointers into the Oilpan heap, as such untraced references risk going stale. With potentially undesirable consequences. Now that we've addressed and handled all such untraced references as part of Blink's transition to Oilpan, it is time to enable the clang GC plugin warning for such raw pointer uses. It shouldn't represent a major imposition to developers to handle such raw pointer uses correctly, but for now we will only emit a warning and not an error. R= BUG=515524 ========== to ========== If Oilpan is enabled, warn of raw heap pointer fields by default. It is unsafe to keep fields with raw pointers into the Oilpan heap, as such untraced references risk going stale. With potentially undesirable consequences. Now that we've addressed and handled all such untraced references as part of Blink's transition to Oilpan, it is time to enable the clang GC plugin warning for such raw pointer uses. It shouldn't represent a major imposition to developers to handle such raw pointer uses correctly, but for now we will only emit a warning and not an error. R=haraken BUG=515524 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464293002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sigbjornf@opera.com
The CQ bit was unchecked by sigbjornf@opera.com
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464293002/1
> It shouldn't represent a major imposition to developers to handle such > raw pointer uses correctly, but for now we will only emit a warning and > not an error. Does -Werror turn these warnings into errors? If not, this not lgtm. We want a noise-free, warning-free build. If warnings are ignorable, people will ignore them, and people will learn that build spam is ok. We worked hard to turn on warnings-as-errors everywhere and to not have any warnings, so let's please not regress this.
On 2015/11/24 18:45:17, Nico wrote: > > It shouldn't represent a major imposition to developers to handle such > > raw pointer uses correctly, but for now we will only emit a warning and > > not an error. > > Does -Werror turn these warnings into errors? If not, this not lgtm. We want a > noise-free, warning-free build. If warnings are ignorable, people will ignore > them, and people will learn that build spam is ok. We worked hard to turn on > warnings-as-errors everywhere and to not have any warnings, so let's please not > regress this. "not lgtm" without asking around a bit first? ok.. We want to move up to always error first/asap..but this is restricted to Oilpan only atm, and we want to take a small step first (and not have to wait on a clang roll etc.)
On 2015/11/24 18:45:17, Nico wrote: > > It shouldn't represent a major imposition to developers to handle such > > raw pointer uses correctly, but for now we will only emit a warning and > > not an error. > > Does -Werror turn these warnings into errors? If not, this not lgtm. We want a > noise-free, warning-free build. If warnings are ignorable, people will ignore > them, and people will learn that build spam is ok. We worked hard to turn on > warnings-as-errors everywhere and to not have any warnings, so let's please not > regress this. (but turning it on as an error sounds fine to me)
On 2015/11/24 18:50:56, sof wrote: > On 2015/11/24 18:45:17, Nico wrote: > > > It shouldn't represent a major imposition to developers to handle such > > > raw pointer uses correctly, but for now we will only emit a warning and > > > not an error. > > > > Does -Werror turn these warnings into errors? If not, this not lgtm. We want a > > noise-free, warning-free build. If warnings are ignorable, people will ignore > > them, and people will learn that build spam is ok. We worked hard to turn on > > warnings-as-errors everywhere and to not have any warnings, so let's please > not > > regress this. > > "not lgtm" without asking around a bit first? ok.. Oh sorry, I didn't realize this comes across as impolite. It just means "please don't land this before this question is answered". > We want to move up to always error first/asap..but this is restricted to Oilpan > only atm, and we want to take a small step first (and not have to wait on a > clang roll etc.) Isn't oilpan on by default at least for some classes by now? If this will only fire on an opt-in build that less than ten people use, then I'm fine with this. Why is this blocked on a clang roll? For https://chromium.googlesource.com/chromium/src/+/c175b30bb17b947d49597d1391a5... ?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/11/24 18:54:07, Nico wrote: > On 2015/11/24 18:50:56, sof wrote: > > On 2015/11/24 18:45:17, Nico wrote: > > > > It shouldn't represent a major imposition to developers to handle such > > > > raw pointer uses correctly, but for now we will only emit a warning and > > > > not an error. > > > > > > Does -Werror turn these warnings into errors? If not, this not lgtm. We want > a > > > noise-free, warning-free build. If warnings are ignorable, people will > ignore > > > them, and people will learn that build spam is ok. We worked hard to turn on > > > warnings-as-errors everywhere and to not have any warnings, so let's please > > not > > > regress this. > > > > "not lgtm" without asking around a bit first? ok.. > > Oh sorry, I didn't realize this comes across as impolite. It just means "please > don't land this before this question is answered". > > > We want to move up to always error first/asap..but this is restricted to > Oilpan > > only atm, and we want to take a small step first (and not have to wait on a > > clang roll etc.) > > Isn't oilpan on by default at least for some classes by now? > Yes, it has been for a while for several script-exposed object. This warning is only defaulted on when building in always-on Oilpan mode (aka enable_oilpan=1), using it on all Oilpan-transitioned objects. > If this will only fire on an opt-in build that less than ten people use, then > I'm fine with this. > > Why is this blocked on a clang roll? For > https://chromium.googlesource.com/chromium/src/+/c175b30bb17b947d49597d1391a5... > ? It isn't, but changing the GC plugin to consider this unsafe raw pointer usage always an error, regardless of -Werror, would require a roll out of the GC plugin.
On 2015/11/24 19:01:33, sof wrote: > On 2015/11/24 18:54:07, Nico wrote: > > On 2015/11/24 18:50:56, sof wrote: > > > On 2015/11/24 18:45:17, Nico wrote: > > > > > It shouldn't represent a major imposition to developers to handle such > > > > > raw pointer uses correctly, but for now we will only emit a warning and > > > > > not an error. > > > > > > > > Does -Werror turn these warnings into errors? If not, this not lgtm. We > want > > a > > > > noise-free, warning-free build. If warnings are ignorable, people will > > ignore > > > > them, and people will learn that build spam is ok. We worked hard to turn > on > > > > warnings-as-errors everywhere and to not have any warnings, so let's > please > > > not > > > > regress this. > > > > > > "not lgtm" without asking around a bit first? ok.. > > > > Oh sorry, I didn't realize this comes across as impolite. It just means > "please > > don't land this before this question is answered". > > > > > We want to move up to always error first/asap..but this is restricted to > > Oilpan > > > only atm, and we want to take a small step first (and not have to wait on a > > > clang roll etc.) > > > > Isn't oilpan on by default at least for some classes by now? > > > > Yes, it has been for a while for several script-exposed object. This warning is > only defaulted on when building in always-on Oilpan mode (aka enable_oilpan=1), > using it on all Oilpan-transitioned objects. Ok, then lgtm in any case. Sorry about the noise. > > > If this will only fire on an opt-in build that less than ten people use, then > > I'm fine with this. > > > > Why is this blocked on a clang roll? For > > > https://chromium.googlesource.com/chromium/src/+/c175b30bb17b947d49597d1391a5... > > ? > > It isn't, but changing the GC plugin to consider this unsafe raw pointer usage > always an error, regardless of -Werror, would require a roll out of the GC > plugin. What I was trying to say above is that if -Werror does map this warning to an error then I'm happy. We always build with -Werror on the bots, so if they stay green with this change it'd mean the warning doesn't get emitted in practice. What I wouldn't like if this was always emitted as a warning, even if -Werror is passed.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464293002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3a192c385e0df065569291963b0aa8a1c992700f Cr-Commit-Position: refs/heads/master@{#361656} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
