Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917603002:
third_party/mesa: turn off ubsan_vptr, since it's compiled without rtti. (Closed)
Descriptionthird_party/mesa: turn off ubsan_vptr, since it's compiled without rtti.
This is a port of a corresponding functionality in GYP:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mesa/mesa.gyp&q=fsanitize=vptr&sq=package:chromium&type=cs&l=84
BUG=606003
Committed: https://crrev.com/6017f972ef987939b1777dc54b37a9c9aecc1b92
Cr-Commit-Position: refs/heads/master@{#390240}
Patch Set 1 #
Total comments: 1
Patch Set 2 : ubsan_vptr_flags config #
Total comments: 2
Patch Set 3 : comments #Patch Set 4 : add missing gn include #Patch Set 5 : implement Brett's proposal #Patch Set 6 : . #
Total comments: 14
Patch Set 7 : address the comments #Patch Set 8 : . #Patch Set 9 : . #Messages
Total messages: 51 (18 generated)
krasin@google.com changed reviewers: + dpranke@google.com
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/1917603002/diff/1/third_party/mesa/BUILD.gn File third_party/mesa/BUILD.gn (right): https://codereview.chromium.org/1917603002/diff/1/third_party/mesa/BUILD.gn#n... third_party/mesa/BUILD.gn:133: cflags += [ "-fno-sanitize=vptr" ] This is dangerous, because this is likely sensitive tot he order in which the flags appear on the command line. The right way to do this is to define a config that sets this flag, have that config get included when is_ubsan_vptr is set to true, and then *remove* that config from this target if need be (so you need to mess with the settings in //build/config/sanitzers/BUILD.gn a bit).
On 2016/04/22 23:04:56, Dirk Pranke wrote: > https://codereview.chromium.org/1917603002/diff/1/third_party/mesa/BUILD.gn > File third_party/mesa/BUILD.gn (right): > > https://codereview.chromium.org/1917603002/diff/1/third_party/mesa/BUILD.gn#n... > third_party/mesa/BUILD.gn:133: cflags += [ "-fno-sanitize=vptr" ] > This is dangerous, because this is likely sensitive tot he order in which the > flags appear on the command line. > > The right way to do this is to define a config that sets this flag, have that > config get included when is_ubsan_vptr is set to true, and then *remove* that > config from this target if need be (so you need to mess with the settings in > //build/config/sanitzers/BUILD.gn a bit). Done. Please, take another look. Note: I am very new to the configs mechanics, it might be that there could be a simpler way to do that. If you see one, please, don't hesitate to suggest.
dpranke@chromium.org changed reviewers: - dpranke@google.com
lgtm, that's the right way to do it. https://codereview.chromium.org/1917603002/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1917603002/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:72: if (is_ubsan_vptr) { maybe add a comment here about why this isn't just a part of :default_sanitizer_flags , i.e., mention the usage in //third_party/mesa ?
https://codereview.chromium.org/1917603002/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1917603002/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:72: if (is_ubsan_vptr) { On 2016/04/26 01:00:32, Dirk Pranke wrote: > maybe add a comment here about why this isn't just a part of > :default_sanitizer_flags , i.e., mention the usage in //third_party/mesa ? Done.
The CQ bit was checked by krasin@google.com
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/1917603002/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917603002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by krasin@google.com
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/1917603002/#ps60001 (title: "add missing gn include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917603002/60001
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...)
krasin@google.com changed reviewers: + kbr@chromium.org
krasin@google.com changed reviewers: + piman@chromium.org - kbr@chromium.org
Antoine, please, approve this tiny BUILD change to fix GN UBSan buildbot.
kbr@chromium.org changed reviewers: + kbr@chromium.org
lgtm
lgtm
The CQ bit was checked by krasin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917603002/60001
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...)
krasin@google.com changed reviewers: + brettw@chromium.org
Hi Brett, can you please LGTM the change of build/config/BUILDCONFIG.gn?
Friendly ping!
Hi Brett, 'UBSanVptr Linux' buildbot being very red right now: https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr%20Linux/builds/41 This CL will fix almost all failures. The remaining failures are handled by another bug (https://crbug.com/605933) Since this CL is already approved by Dirk, it's just one line in build/config/BUILDCONFIG.gn that needs your review. Can you please take a look? I am really uncomfortable to have the buildbot red and the fix ready for this long.
@krasin: I lgtm'ed this a while ago, but it still lgtm :).
On 2016/04/27 18:03:12, Dirk Pranke wrote: > @krasin: I lgtm'ed this a while ago, but it still lgtm :). brettw@ is configured as the only allowed reviewer of build/config/BUILDCONFIG.gn. @krasin: I suggest TBR'ing brettw.
On 2016/04/27 18:06:19, Ken Russell OOO till 5-2-2016 wrote: > On 2016/04/27 18:03:12, Dirk Pranke wrote: > > @krasin: I lgtm'ed this a while ago, but it still lgtm :). > > brettw@ is configured as the only allowed reviewer of > build/config/BUILDCONFIG.gn. Never TBR for this file.
So I hate adding global flags to BUILDCONFIG since it makes everything global
and non-modular. I feel like this pattern, especially in this case, leads us
down a path where all of the sanitizer configs will eventually be listed in
BUILDCONFIG and it will be a big mess.
I wonder about the following approach:
Break out the individual parts of default_sanitizer_configs into sub-configs.
Some of this is already done and it seems like a good idea from a cleanup
perspective anyway.
So we end up with a tsan one, a cfi one, a ubsan one (one for each condition in
the default flags config). Now default_sanitizer_configs entire contents is:
configs = [ ":ubsan", ":lsan", ... ]
Now we can easily make other composite configs in that file for specialized uses
("everything but ubsan_ptr", "everything but coverage") or targets can opt-into
exactly what subset they want at a very fine grain after removing the "default"
ones.
To prevent skew, one could even express the list of configs in that file like:
all_sanitizer_configs = [ ... ]
config("default_sanitizer_configs") {
configs = all_sanitizer_configs
}
config("everything_but_ubsan_ptr") {
configs = all_sanitizer_configs - ":ubsan_ptr" ]
}
...
On 2016/04/27 18:21:37, brettw wrote:
> So I hate adding global flags to BUILDCONFIG since it makes everything global
> and non-modular. I feel like this pattern, especially in this case, leads us
> down a path where all of the sanitizer configs will eventually be listed in
> BUILDCONFIG and it will be a big mess.
>
> I wonder about the following approach:
>
> Break out the individual parts of default_sanitizer_configs into sub-configs.
> Some of this is already done and it seems like a good idea from a cleanup
> perspective anyway.
>
> So we end up with a tsan one, a cfi one, a ubsan one (one for each condition
in
> the default flags config). Now default_sanitizer_configs entire contents is:
> configs = [ ":ubsan", ":lsan", ... ]
>
> Now we can easily make other composite configs in that file for specialized
uses
> ("everything but ubsan_ptr", "everything but coverage") or targets can
opt-into
> exactly what subset they want at a very fine grain after removing the
"default"
> ones.
>
> To prevent skew, one could even express the list of configs in that file like:
> all_sanitizer_configs = [ ... ]
> config("default_sanitizer_configs") {
> configs = all_sanitizer_configs
> }
> config("everything_but_ubsan_ptr") {
> configs = all_sanitizer_configs - ":ubsan_ptr" ]
> }
> ...
Just to make sure I follow the proposal. In the current CL, I have the following
changes in third_party/mesa/BUILD.gn:
if (is_ubsan_vptr) {
# UBsan's vptr is not compatible with -fno-rtti,
# which is used by gallium/auxiliary/Makefile.
configs -= [ "//build/config/sanitizers:ubsan_vptr_flags" ]
}
With you suggestion it will become:
if (is_ubsan_vptr) {
# UBsan's vptr is not compatible with -fno-rtti,
# which is used by gallium/auxiliary/Makefile.
configs -= [ "//build/config/sanitizers:all_sanitizers_configs" ]
configs += [ "//build/config/sanitizers:everything_but_ubsan_vptr" ]
}
Is that correct?
On 2016/04/27 18:28:31, krasin wrote:
> On 2016/04/27 18:21:37, brettw wrote:
> > So I hate adding global flags to BUILDCONFIG since it makes everything
global
> > and non-modular. I feel like this pattern, especially in this case, leads us
> > down a path where all of the sanitizer configs will eventually be listed in
> > BUILDCONFIG and it will be a big mess.
> >
> > I wonder about the following approach:
> >
> > Break out the individual parts of default_sanitizer_configs into
sub-configs.
> > Some of this is already done and it seems like a good idea from a cleanup
> > perspective anyway.
> >
> > So we end up with a tsan one, a cfi one, a ubsan one (one for each condition
> in
> > the default flags config). Now default_sanitizer_configs entire contents is:
> > configs = [ ":ubsan", ":lsan", ... ]
> >
> > Now we can easily make other composite configs in that file for specialized
> uses
> > ("everything but ubsan_ptr", "everything but coverage") or targets can
> opt-into
> > exactly what subset they want at a very fine grain after removing the
> "default"
> > ones.
> >
> > To prevent skew, one could even express the list of configs in that file
like:
> > all_sanitizer_configs = [ ... ]
> > config("default_sanitizer_configs") {
> > configs = all_sanitizer_configs
> > }
> > config("everything_but_ubsan_ptr") {
> > configs = all_sanitizer_configs - ":ubsan_ptr" ]
> > }
> > ...
>
> Just to make sure I follow the proposal. In the current CL, I have the
following
> changes in third_party/mesa/BUILD.gn:
>
> if (is_ubsan_vptr) {
> # UBsan's vptr is not compatible with -fno-rtti,
> # which is used by gallium/auxiliary/Makefile.
> configs -= [ "//build/config/sanitizers:ubsan_vptr_flags" ]
> }
>
> With you suggestion it will become:
>
> if (is_ubsan_vptr) {
> # UBsan's vptr is not compatible with -fno-rtti,
> # which is used by gallium/auxiliary/Makefile.
> configs -= [ "//build/config/sanitizers:all_sanitizers_configs" ]
> configs += [ "//build/config/sanitizers:everything_but_ubsan_vptr" ]
> }
>
> Is that correct?
Yes, basically.
I would keep the current "default_sanitizers" name for the default one.
And I think you should be able to skip the if condition in both places. These
configs would always be applied, but internally each config would switch its
flags on and off according to whether that thing is enabled for the build (as
the default one already does). This allows us to centralize the logic and make
the individual build files cleaner.
> >
> > if (is_ubsan_vptr) {
> > # UBsan's vptr is not compatible with -fno-rtti,
> > # which is used by gallium/auxiliary/Makefile.
> > configs -= [ "//build/config/sanitizers:all_sanitizers_configs" ]
> > configs += [ "//build/config/sanitizers:everything_but_ubsan_vptr" ]
> > }
> >
> > Is that correct?
>
> Yes, basically.
>
> I would keep the current "default_sanitizers" name for the default one.
>
> And I think you should be able to skip the if condition in both places. These
> configs would always be applied, but internally each config would switch its
> flags on and off according to whether that thing is enabled for the build (as
> the default one already does). This allows us to centralize the logic and make
> the individual build files cleaner.
Okay, I see. What if some code will need to be compiled without two kinds of
sanitizers, like ubsan_vptr and cfi?
Will I have to add //build/config/sanitizers:everything_but_ubsan_vptr_and_cfi?
Not that I have a need right now, just to make sure we're not surprised, when we
find tens of different sanitizers configs which are essentially bitmasks.
On 2016/04/27 19:42:58, krasin wrote: > Okay, I see. What if some code will need to be compiled without two kinds of > sanitizers, like ubsan_vptr and cfi? > Will I have to add //build/config/sanitizers:everything_but_ubsan_vptr_and_cfi? > > Not that I have a need right now, just to make sure we're not surprised, when we > find tens of different sanitizers configs which are essentially bitmasks. I was thinking we'll worry about that when we get there. So far, we have two examples of code needing to opt-out of one specific thing. I would hope that if we start getting a lot of these exceptions, everything will fall into that, it wants no sanitizers at all, or maybe there will be 1-2 classes of sanitizer things such cases will fall into.
> I was thinking we'll worry about that when we get there. So far, we have two > examples of code needing to opt-out of one specific thing. I would hope that if > we start getting a lot of these exceptions, everything will fall into that, it > wants no sanitizers at all, or maybe there will be 1-2 classes of sanitizer > things such cases will fall into. Okay, sounds good. Will do!
I have implemented Brett's proposal. Brett, Dirk, please take a look.
LGTM, thanks. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:197: defines = [] This needs a cflags = [] https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:230: cflags = [] In this case and a few below when it's only set once and nothing else happens, I think it's better to just do "cflags = []" once and not do the empty one at the top and append later pattern. GN will tell you if you clobber a nonempty list with another nonempty list, which effectively eliminates mistakes. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:249: cflags += [ Ditto, just = and delete the empty list at the top. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:301: cflags += [ This should be "=" https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:344: # //third_party/mesa has some code compiled without rtti, which requires I'd actually not bother mentioning where this is needed. Such things tend to get out-of-date and it's better just to check for users to see what they want. It might be nice to add a bit more background about why one might need to opt-out of ubsan_ptr. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:362: defines = [] You can keep this config the way it was. https://codereview.chromium.org/1917603002/diff/100001/third_party/mesa/READM... File third_party/mesa/README.chromium (right): https://codereview.chromium.org/1917603002/diff/100001/third_party/mesa/READM... third_party/mesa/README.chromium:73: - GN: disabled ubsan_vptr, since ubsan_vptr requires RTTI. I think the BUILD.gn file is ours rather than forked from upstream, so I don't think we actually need a README for this.
All done. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:197: defines = [] On 2016/04/27 22:03:23, brettw wrote: > This needs a cflags = [] Done. Thank you for the catch. I've removed += from defines, as they are set in a single place. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:230: cflags = [] On 2016/04/27 22:03:23, brettw wrote: > In this case and a few below when it's only set once and nothing else happens, I > think it's better to just do "cflags = []" once and not do the empty one at the > top and append later pattern. GN will tell you if you clobber a nonempty list > with another nonempty list, which effectively eliminates mistakes. Done. Thanks, that's a useful warning. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:249: cflags += [ On 2016/04/27 22:03:23, brettw wrote: > Ditto, just = and delete the empty list at the top. Done. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:301: cflags += [ On 2016/04/27 22:03:23, brettw wrote: > This should be "=" Done. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:344: # //third_party/mesa has some code compiled without rtti, which requires On 2016/04/27 22:03:23, brettw wrote: > I'd actually not bother mentioning where this is needed. Such things tend to get > out-of-date and it's better just to check for users to see what they want. It > might be nice to add a bit more background about why one might need to opt-out > of ubsan_ptr. I've rephrased the comment. https://codereview.chromium.org/1917603002/diff/100001/build/config/sanitizer... build/config/sanitizers/BUILD.gn:362: defines = [] On 2016/04/27 22:03:23, brettw wrote: > You can keep this config the way it was. Done. https://codereview.chromium.org/1917603002/diff/100001/third_party/mesa/READM... File third_party/mesa/README.chromium (right): https://codereview.chromium.org/1917603002/diff/100001/third_party/mesa/READM... third_party/mesa/README.chromium:73: - GN: disabled ubsan_vptr, since ubsan_vptr requires RTTI. On 2016/04/27 22:03:24, brettw wrote: > I think the BUILD.gn file is ours rather than forked from upstream, so I don't > think we actually need a README for this. Done.
The CQ bit was checked by krasin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, piman@chromium.org, kbr@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1917603002/#ps160001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917603002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917603002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6017f972ef987939b1777dc54b37a9c9aecc1b92 Cr-Commit-Position: refs/heads/master@{#390240}
Message was sent while issue was closed.
Description was changed from ========== third_party/mesa: turn off ubsan_vptr, since it's compiled without rtti. This is a port of a corresponding functionality in GYP: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mesa/m... BUG=606003 ========== to ========== third_party/mesa: turn off ubsan_vptr, since it's compiled without rtti. This is a port of a corresponding functionality in GYP: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mesa/m... BUG=606003 Committed: https://crrev.com/6017f972ef987939b1777dc54b37a9c9aecc1b92 Cr-Commit-Position: refs/heads/master@{#390240} ========== |
