Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10854031:
Support vtable_verify (new compiler option). (Closed)
|
Created:
8 years, 4 months ago by cmtice Modified:
8 years, 4 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSupport vtable_verify (new compiler option).
I'm in the process of adding a new option to the gcc compiler in ChromeOS, that does vtable pointer verification before virtual method calls. The new compiler flag is "-fvtable-verify=std" or "-fvtable-verify=preinit". I'm modifying the Chrome ebuild flag in Chrome OS to allow "USE=vtable_verify" and which causes 'use_vtable_verify' to be defined and set to 1.
In this CL I have modified common.gypi to add the compiler flag '-fvtable-verify=std' if it sees use_vtable_verify set to 1; and I have modified allocator.gyp to change '-fvtable-verify=std' into '-fvtable-verify=preinit' (tcmalloc stuff requires this special flag).
BUG=
TEST=Built Chrome with GYP_DEFINES=use_vtable_verify=1
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152003
Patch Set 1 #Patch Set 2 : Do not turn on verification by default. #
Total comments: 4
Patch Set 3 : Removed common.gypi changes; further discussion led to conclusion it's not needed. #
Total comments: 5
Patch Set 4 : Add default definition of use_vtable_verify. #
Total comments: 2
Patch Set 5 : Remove useless, unneeded code from allocator.gyp #
Total comments: 1
Patch Set 6 : Remove redundant flags. #Messages
Total messages: 27 (0 generated)
Please review this CL. It is needed for a new compiler feature, vtable pointer verification.
On 2012/08/07 20:59:15, cmtice wrote: > Please review this CL. It is needed for a new compiler feature, vtable pointer > verification. i'm removing myself as a reviewer. i'm not familiar with this and i'm not an owner in these directories
Added a few more reviewers in the hopes of finding someone who is 'appropriate'. :-)
Where can I read about this feature and its code impact? Why is this not debug-only?
cmtice@google.com On Tue, Aug 7, 2012 at 5:47 PM, <mark@chromium.org> wrote: > Where can I read about this feature and its code impact? > There is a feature design document I can share with you if you wish to read it. > Why is this not debug-only? This is not a debugging feature. This is not a feature to find programmer errors; it is to detect and handle malicious attacks. In some ways it is similar to the stack-protector compiler feature. > > http://codereview.chromium.org/10854031/
I modified the patch slightly and have uploaded the new patchset to this CL. The original patch always turned on the verification options for Linux or ChromeOS. The new patch removed those lines, so verification is only turned on if explicitly requested.
http://codereview.chromium.org/10854031/diff/4003/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/4003/base/allocator/allocator.gy... base/allocator/allocator.gyp:398: 'cflags': [ It is normal in GYP conditions like this to only have this indent bump in by 2 spaces relative to the preceding line, not 4 as you’ve written. This is handled on the other end by putting the } and ] on the same line, as you did on line 406. http://codereview.chromium.org/10854031/diff/4003/base/allocator/allocator.gy... base/allocator/allocator.gyp:405: ], Fix weird indentation. This should line up with the 'cflags!' two lines above. http://codereview.chromium.org/10854031/diff/4003/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10854031/diff/4003/build/common.gypi#newcode390 build/common.gypi:390: # Whether or not to turn on vtable verification hardening. Do you need this? Can you get rid of it and move the comment and default 0 value down to the enclosing scope? http://codereview.chromium.org/10854031/diff/4003/build/common.gypi#newcode1228 build/common.gypi:1228: 'debug_extra_cflags': '-fvtable-verify=std', Fix up weird indentation here too.
New patch set uploaded. I made requested format changes in allocator.gyp. I removed common.gypi from this CL; further discussion with my group led to the conclusion it wasn't needed.
http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... base/allocator/allocator.gyp:397: [ 'use_vtable_verify==1', { Since you’ve removed use_vtable_verify from common.gypi AND you haven’t provided a default definition anywhere here, this will now cause gyp to throw an exception (except in the rare and unexpected case where a user has affirmatively set this GYP variable manually in GYP_DEFINES, ~/.gyp/include.gypi, or -D.)
http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... base/allocator/allocator.gyp:404: '-fvtable-verify=std', If nothing is adding this option any more, you can remove the cflags! that excludes it.
use_vtable_verify is now being defined in the Chrome ebuild file in ChromeOS (in the version I am planning on committing when the compiler changes go into ChromeOS). I am not sure if I need to define use_vtable_verify in allocator.gyp, to prevent the exception you mentioned; I am a complete novice at gyp stuff (I work almost entirely on maintaining gcc & gdb and I never saw gyp files before), so any advice on the best/correct way to do this would be appreciated. -- Caroline Tice cmtice@chromium.org On Tue, Aug 14, 2012 at 2:32 PM, <mark@chromium.org> wrote: > > http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... > base/allocator/allocator.gyp:404: '-fvtable-verify=std', > If nothing is adding this option any more, you can remove the cflags! > that excludes it. > > http://codereview.chromium.org/10854031/
What I really *wanted* to do was to have allocator.gyp detect if the cflags contains '-fvtable-verify=std', and if so, in the case of the allocator, to replace it with '-fvtable-verify=preinit'. It was suggested to me that the best way to do this was to define use_vtable_verify when the '-fvtable-verify=std' flag was used, and then the gyp file could test for that and change it, as in this CL. Is there way to test the contents of cflags directly, without having to bother with something like 'use_vtable_verify'? -- Caroline On Tue, Aug 14, 2012 at 4:49 PM, Caroline Tice <cmtice@chromium.org> wrote: > use_vtable_verify is now being defined in the Chrome ebuild file in > ChromeOS (in the version I am planning on committing when the compiler > changes go into ChromeOS). > > I am not sure if I need to define use_vtable_verify in allocator.gyp, > to prevent the exception you mentioned; I am a complete novice at gyp > stuff (I work almost entirely on maintaining gcc & gdb and I never saw > gyp files before), so any advice on the best/correct way to do this > would be appreciated. > > -- Caroline Tice > cmtice@chromium.org > > On Tue, Aug 14, 2012 at 2:32 PM, <mark@chromium.org> wrote: >> >> http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gyp >> File base/allocator/allocator.gyp (right): >> >> http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... >> base/allocator/allocator.gyp:404: '-fvtable-verify=std', >> If nothing is adding this option any more, you can remove the cflags! >> that excludes it. >> >> http://codereview.chromium.org/10854031/
http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... base/allocator/allocator.gyp:6: 'variables': { This is a good variables section to use for your default definition (see below). http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... base/allocator/allocator.gyp:397: [ 'use_vtable_verify==1', { You do need to provide a default definition of use_vtable_verify just as you had done before in common.gypi. It can be in this file. It should look something like 'variables': { 'use_vtable_verify%': 0, }, A run through the Chromium try servers should tell you whether you’ve gotten this right, since as I mentioned, nothing elsewhere will be setting this GYP variable. http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... base/allocator/allocator.gyp:404: '-fvtable-verify=std', How would cflags ever contain -fvtable-verify=std? Nothing anywhere in the GYP files ever specifies it.
On Tue, Aug 14, 2012 at 5:18 PM, <mark@chromium.org> wrote: > > http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... > base/allocator/allocator.gyp:6: 'variables': { > This is a good variables section to use for your default definition (see > below). > > Ok, I'll do that, thanks! > http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... > base/allocator/allocator.gyp:397: [ 'use_vtable_verify==1', { > You do need to provide a default definition of use_vtable_verify just as > you had done before in common.gypi. It can be in this file. It should > look something like > > 'variables': { > 'use_vtable_verify%': 0, > }, > > A run through the Chromium try servers should tell you whether you’ve > gotten this right, since as I mentioned, nothing elsewhere will be > setting this GYP variable. > > When I try 'git try', the command fails without actually doing anything: $ git try Loaded authentication cookies from /home/cmtice/.codereview_upload_cookies .gclient file in parent directory /usr/local/google/home/cmtice/chrome-src might not be the file you want to use Results will be emailed to: cmtice@chromium.org Password for '(null)' GNOME keyring: svn: GNOME Keyring is locked and we are non-interactive Command svn import -q /tmp/tmpO3KLNQ svn://svn.chromium.org/chrome-try/try --file /tmp/tmp2DXUxy --no-ignore returned non-zero exit status 1 Sorry, Tryserver is not available. $ Do you have any idea what's going wrong here or how I should fix it? > http://codereview.chromium.org/10854031/diff/5002/base/allocator/allocator.gy... > base/allocator/allocator.gyp:404: '-fvtable-verify=std', > How would cflags ever contain -fvtable-verify=std? Nothing anywhere in > the GYP files ever specifies it. > "-fvtable-verify=std" is being added automatically to the compiler flags for Chrome, in the gcc wrapper script (in my test environment, for now; eventually it will be a default flag for everybody). > http://codereview.chromium.org/10854031/
Caroline Tice wrote: > $ git try > Loaded authentication cookies from /home/cmtice/.codereview_upload_cookies > .gclient file in parent directory > /usr/local/google/home/cmtice/chrome-src might not be the file you > want to use > Results will be emailed to: cmtice@chromium.org > Password for '(null)' GNOME keyring: > svn: GNOME Keyring is locked and we are non-interactive > Command svn import -q /tmp/tmpO3KLNQ > svn://svn.chromium.org/chrome-try/try --file /tmp/tmp2DXUxy > --no-ignore returned non-zero exit status 1 > Sorry, Tryserver is not available. > $ > > Do you have any idea what's going wrong here or how I should fix it? Looks like it’s having a hard time with a keyring, or trying to use the wrong keyring. Make sure you can access svn://svn.chromium.org/chrome-trywith the correct username and password. It may only work properly from the context of a graphical session. Also look at [auth]password-stores .subversion/config—you can try to remove gnome-keyring to avoid using gnome-keyring (and with [auth]store-passwords=no, you’d hope to be prompted for the svn password each time it’s needed.) "-fvtable-verify=std" is being added automatically to the compiler > flags for Chrome, in the gcc wrapper script (in my test environment, > for now; eventually it will be a default flag for everybody) > The “gcc wrapper script” sounds like something that the build system invokes instead of gcc. Is that right? If so, and if it’s setting -fvtable-verify=std, then -fvtable-verify=std won’t ever appear to GYP in cflags. GYP can only remove things that it knows about. If you’ve got a wrapper that does something like “exec gcc.real -fvtable-verify=std ${@}", then GYP or make or any other build system wouldn’t be able to prevent -fvtable-verify=std from being passed to gcc.real.
I've uploaded a new patch set that adds a default definition of use_vtable_verify. I managed to fix the problems I was having with 'git try', so I've done that now (I actually did it twice, once before I uploaded the new patch and once after). use_vtable_verify will be set to 1 by the Chrome ebuild file (in ChromeOS), when it's appropriate.
http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.g... base/allocator/allocator.gyp:405: '-fvtable-verify=std', OK, so let’s talk more about this. Where’s your “gcc wrapper script” sit in the pipeline?
On Wed, Aug 15, 2012 at 1:21 PM, <mark@chromium.org> wrote: > > http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.g... > base/allocator/allocator.gyp:405: '-fvtable-verify=std', > OK, so let’s talk more about this. Where’s your “gcc wrapper script” sit > in the pipeline? > When you invoke 'gcc' or 'g++' in ChromeOS the gcc wrapper script is what actually gets called. It parses some of the flags and calls the gcc binaries (and then the assembler and the linker, and passes the linker flags to the linker rather than the compiler binary, etc.). You don't do anything special or new, this is the way it's always worked. -- Caroline Tice cmtice@chromium.org > http://codereview.chromium.org/10854031/
http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.g... base/allocator/allocator.gyp:405: '-fvtable-verify=std', So then this line, which is supposed to exclude -fvtable-verify=std from cflags, is entirely inoperative. Your wrapper script is passing -fvtable-verify=std to the actual compiler driver. -fvtable-verify=std is never present in the configuration in GYP or in the build environment (make, ninja, or whatever else).
On 2012/08/15 20:38:36, Mark Mentovai wrote: > http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.g... > base/allocator/allocator.gyp:405: '-fvtable-verify=std', > So then this line, which is supposed to exclude -fvtable-verify=std from cflags, > is entirely inoperative. Your wrapper script is passing -fvtable-verify=std to > the actual compiler driver. -fvtable-verify=std is never present in the > configuration in GYP or in the build environment (make, ninja, or whatever > else). Mark, is there a way for gyp to do a check at runtime (like compile a small file with $CXX -v) and look at the flags in stdout/err and then add a -fvtable-verify=preinit? Sounds like that's what we want here. The other approach would be not to put this flag in the driver and put it instead of C[XX]FLAGS and make sure those are passed into the env of gyp as RELEASE_EXTRA_CFLAGS, etc. Though with this approach, if cmtice makes sure that the wrapper change is submitted atomically with a change in the chrome ebuild that passes in use_vtable_verify=1, this change should be fine, right?
asharif@chromium.org wrote: > Mark, is there a way for gyp to do a check at runtime (like compile a > small file > with $CXX -v) and look at the flags in stdout/err and then add a > -fvtable-verify=preinit? Sounds like that's what we want here. Not unless you write it yourself. I don’t think that’s what you want here anyway. > The other approach would be not to put this flag in the driver and put it > instead of C[XX]FLAGS and make sure those are passed into the env of gyp as > RELEASE_EXTRA_CFLAGS, etc. > > Though with this approach, if cmtice makes sure that the wrapper change is > submitted atomically with a change in the chrome ebuild that passes in > use_vtable_verify=1, this change should be fine, right? > If you have a wrapper that sets -fvtable-verify=something, then I think the right solution is to make that wrapper check whether it was invoked with -fvtable-verify=something-else, and if so, avoid setting -fvtable-verify=something and pass on -fvtable-verify=something-else. The cflags! that I see in this change are ineffective.
On Wed, Aug 15, 2012 at 1:38 PM, <mark@chromium.org> wrote: > > http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > http://codereview.chromium.org/10854031/diff/15002/base/allocator/allocator.g... > base/allocator/allocator.gyp:405: '-fvtable-verify=std', > So then this line, which is supposed to exclude -fvtable-verify=std from > cflags, is entirely inoperative. Your wrapper script is passing > -fvtable-verify=std to the actual compiler driver. -fvtable-verify=std > is never present in the configuration in GYP or in the build environment > (make, ninja, or whatever else). > > http://codereview.chromium.org/10854031/ Ok, I just finished quite a bit of testing and have a much better understanding of how this is working. In ChromeOS I have modified the gcc wrapper script so that it automatically passes "-fvtable-verify=std" to the compiler binary. I have also modified the Chrome ebuild file to define 'use_vtable_verify = 1'. When the allocator files get built, using the modified gyp file in my patch, the '-ftable-verify=std' is being passed to the compiler binary fairly early in the command line (and invisibly to log files, etc.). The use_vtable_verify variable comes in with a value of '1', courtesy of the Chrome ebuild file, so my condition triggers and adds "-fvtable-verify=preinit" to the compiler flags. You are correct, the code I have in the gyp file that was attempting to remove "-fvtable-verify=std" was not doing anything. But the "-fvtable-verify=preinit" is being added to the compiler command line in a later position that the '-fvtable-verify=std', which means the preinit version is the one that actually gets used, which is the behavior I want. I have modified the patch to removed the useless bit of code that was trying to remove -fvtable-verify=std, and I have uploaded the new patch to the CL.
http://codereview.chromium.org/10854031/diff/1004/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10854031/diff/1004/base/allocator/allocator.gy... base/allocator/allocator.gyp:400: '-Wno-sign-compare', OS==linux already sets -Wno-sign-compare and -Wno-unused-result above on lines 381 and 382. These two are extraneous here. cflags are additive. You can remove these.
New patch set uploaded; redundant cflags removed. On Thu, Aug 16, 2012 at 11:05 AM, <mark@chromium.org> wrote: > > http://codereview.chromium.org/10854031/diff/1004/base/allocator/allocator.gyp > File base/allocator/allocator.gyp (right): > > http://codereview.chromium.org/10854031/diff/1004/base/allocator/allocator.gy... > base/allocator/allocator.gyp:400: '-Wno-sign-compare', > OS==linux already sets -Wno-sign-compare and -Wno-unused-result above on > lines 381 and 382. These two are extraneous here. cflags are additive. > You can remove these. > > http://codereview.chromium.org/10854031/
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmtice@chromium.org/10854031/12003
Change committed as 152003 |
