Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
605623002:
Enable debug fission for Release build (Closed)
DescriptionEnable debug fission for Release build
This enables debug fission for Release build. If the target compiler
satisfies fission requirement, the fission is enabled for target no
matter how old the host compiler is.
BUG=None
TEST=simple chrome for x64 platform passes with fission.
Committed: https://crrev.com/d872dd066eb644f2d85e25429f55451cd2dd20d4
Cr-Commit-Position: refs/heads/master@{#298925}
Patch Set 1 #
Total comments: 16
Messages
Total messages: 23 (7 generated)
yunlian@chromium.org changed reviewers: + tansell@chromium.org
mithro@mithis.com changed reviewers: + mithro@mithis.com
Hi Yunlian, What does your GYP_DEFINEs and other similar settings look like? I wasn't able to track down the exact reason for your issue but I went through and added a whole bunch of comments for locations in the file which might have relevant information to work from. Getting things at the right level in GYP is a bit of a black art. Hope that helps, https://codereview.chromium.org/605623002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode841 build/common.gypi:841: ['OS=="linux" and (target_arch=="x64" or target_arch=="arm")', { This is whether to use bundled gold by default. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode851 build/common.gypi:851: # are using a custom toolchain and need to control -B in cflags. This is whether to use bundled binutils by default. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode859 build/common.gypi:859: # On by default for x64 Linux. This is whether to use gold *flags* by default. NFI Why this isn't just looking to see if the linker is going to be gold..... https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode870 build/common.gypi:870: # http://gcc.gnu.org/wiki/DebugFission This is whether to enable debug fission by default. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1159 build/common.gypi:1159: 'linux_use_debug_fission%': '<(linux_use_debug_fission)', This is where we override the defaults from the environment. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1516 build/common.gypi:1516: ['os_posix==1 and OS!="mac" and OS!="ios"', { This is where binutils version information is populated. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1549 build/common.gypi:1549: ['os_posix==1 and OS!="mac" and OS!="ios" and clang==0 and asan==0 and lsan==0 and tsan==0 and msan==0 and ubsan_vptr==0', { This is where gcc version information is populated. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1608 build/common.gypi:1608: # and a 64-bit libc installed. There seems to be some type of special override here? https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1840 build/common.gypi:1840: 'clang%': 1, Not sure what is going on here.... https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3506 build/common.gypi:3506: ['os_posix==1 and OS!="mac" and OS!="ios"', { Check this bit here. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3606 build/common.gypi:3606: ['linux_use_debug_fission==1 and linux_use_gold_flags==1 and (clang==1 or gcc_version>=48) and binutils_version>=223', { Check this is working for you under debug builds. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3642 build/common.gypi:3642: 'conditions' : [ Can you test your conditions in this section rather than your current location? https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3686 build/common.gypi:3686: }, This is popped up a couple of levels, there could be something to that.... https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode4338 build/common.gypi:4338: }], This is a bunch of extra stuff based on the binutils flags. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode4365 build/common.gypi:4365: }], This is a bunch of extra stuff based on the version information. https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode5644 build/common.gypi:5644: # TODO: remove this flag once all builds work. See crbug.com/227506 Some more conditions based on gcc_version and similar.
On 2014/09/25 04:51:09, mithro wrote: > Hi Yunlian, > > What does your GYP_DEFINEs and other similar settings look like? > > I wasn't able to track down the exact reason for your issue but I went through > and added a whole bunch of comments for locations in the file which might have > relevant information to work from. > > Getting things at the right level in GYP is a bit of a black art. > > Hope that helps, > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode841 > build/common.gypi:841: ['OS=="linux" and (target_arch=="x64" or > target_arch=="arm")', { > This is whether to use bundled gold by default. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode851 > build/common.gypi:851: # are using a custom toolchain and need to control -B in > cflags. > This is whether to use bundled binutils by default. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode859 > build/common.gypi:859: # On by default for x64 Linux. > This is whether to use gold *flags* by default. > > NFI Why this isn't just looking to see if the linker is going to be gold..... > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode870 > build/common.gypi:870: # http://gcc.gnu.org/wiki/DebugFission > This is whether to enable debug fission by default. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1159 > build/common.gypi:1159: 'linux_use_debug_fission%': > '<(linux_use_debug_fission)', > This is where we override the defaults from the environment. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1516 > build/common.gypi:1516: ['os_posix==1 and OS!="mac" and OS!="ios"', { > This is where binutils version information is populated. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1549 > build/common.gypi:1549: ['os_posix==1 and OS!="mac" and OS!="ios" and clang==0 > and asan==0 and lsan==0 and tsan==0 and msan==0 and ubsan_vptr==0', { > This is where gcc version information is populated. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1608 > build/common.gypi:1608: # and a 64-bit libc installed. > There seems to be some type of special override here? > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1840 > build/common.gypi:1840: 'clang%': 1, > Not sure what is going on here.... > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3506 > build/common.gypi:3506: ['os_posix==1 and OS!="mac" and OS!="ios"', { > Check this bit here. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3606 > build/common.gypi:3606: ['linux_use_debug_fission==1 and linux_use_gold_flags==1 > and (clang==1 or gcc_version>=48) and binutils_version>=223', { > Check this is working for you under debug builds. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3642 > build/common.gypi:3642: 'conditions' : [ > Can you test your conditions in this section rather than your current location? > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3686 > build/common.gypi:3686: }, > This is popped up a couple of levels, there could be something to that.... > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode4338 > build/common.gypi:4338: }], > This is a bunch of extra stuff based on the binutils flags. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode4365 > build/common.gypi:4365: }], > This is a bunch of extra stuff based on the version information. > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode5644 > build/common.gypi:5644: # TODO: remove this flag once all builds work. See > crbug.com/227506 > Some more conditions based on gcc_version and similar. It seems that there is GYP_DEFINES inside the chrome_sdk, and the fission_debug is disabled there, after reseting the linux_debug_fission variable, my patch works fine.
On 2014/09/26 06:03:46, yunlian wrote: > On 2014/09/25 04:51:09, mithro wrote: > > Hi Yunlian, > > > > What does your GYP_DEFINEs and other similar settings look like? > > > > I wasn't able to track down the exact reason for your issue but I went through > > and added a whole bunch of comments for locations in the file which might have > > relevant information to work from. > > > > Getting things at the right level in GYP is a bit of a black art. > > > > Hope that helps, > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi > > File build/common.gypi (right): > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode841 > > build/common.gypi:841: ['OS=="linux" and (target_arch=="x64" or > > target_arch=="arm")', { > > This is whether to use bundled gold by default. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode851 > > build/common.gypi:851: # are using a custom toolchain and need to control -B > in > > cflags. > > This is whether to use bundled binutils by default. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode859 > > build/common.gypi:859: # On by default for x64 Linux. > > This is whether to use gold *flags* by default. > > > > NFI Why this isn't just looking to see if the linker is going to be gold..... > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode870 > > build/common.gypi:870: # http://gcc.gnu.org/wiki/DebugFission > > This is whether to enable debug fission by default. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1159 > > build/common.gypi:1159: 'linux_use_debug_fission%': > > '<(linux_use_debug_fission)', > > This is where we override the defaults from the environment. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1516 > > build/common.gypi:1516: ['os_posix==1 and OS!="mac" and OS!="ios"', { > > This is where binutils version information is populated. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1549 > > build/common.gypi:1549: ['os_posix==1 and OS!="mac" and OS!="ios" and clang==0 > > and asan==0 and lsan==0 and tsan==0 and msan==0 and ubsan_vptr==0', { > > This is where gcc version information is populated. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1608 > > build/common.gypi:1608: # and a 64-bit libc installed. > > There seems to be some type of special override here? > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode1840 > > build/common.gypi:1840: 'clang%': 1, > > Not sure what is going on here.... > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3506 > > build/common.gypi:3506: ['os_posix==1 and OS!="mac" and OS!="ios"', { > > Check this bit here. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3606 > > build/common.gypi:3606: ['linux_use_debug_fission==1 and > linux_use_gold_flags==1 > > and (clang==1 or gcc_version>=48) and binutils_version>=223', { > > Check this is working for you under debug builds. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3642 > > build/common.gypi:3642: 'conditions' : [ > > Can you test your conditions in this section rather than your current > location? > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode3686 > > build/common.gypi:3686: }, > > This is popped up a couple of levels, there could be something to that.... > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode4338 > > build/common.gypi:4338: }], > > This is a bunch of extra stuff based on the binutils flags. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode4365 > > build/common.gypi:4365: }], > > This is a bunch of extra stuff based on the version information. > > > > https://codereview.chromium.org/605623002/diff/1/build/common.gypi#newcode5644 > > build/common.gypi:5644: # TODO: remove this flag once all builds work. See > > crbug.com/227506 > > Some more conditions based on gcc_version and similar. > > It seems that there is GYP_DEFINES inside the chrome_sdk, and the fission_debug > is disabled there, after reseting the linux_debug_fission variable, > my patch works fine. Ping? Is it good got land?
LGTM from a common.gypi point of view, but could you please get someone from ChromeOS to give you a LGTM too before landing. Tim
yunlian@chromium.org changed reviewers: + llozano@chromium.org
llozano@google.com changed reviewers: + llozano@google.com
lgtm this looks ok to me.
The CQ bit was checked by yunlian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/605623002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by yunlian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/605623002/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/d872dd066eb644f2d85e25429f55451cd2dd20d4 Cr-Commit-Position: refs/heads/master@{#298925}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/649433002/ by maniscalco@chromium.org. The reason for reverting is: Speculatively reverting. Suspect it may be cause of leaks on linux asan lsan bot: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... .
Message was sent while issue was closed.
On 2014/10/09 22:45:52, maniscalco wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.chromium.org/649433002/ by mailto:maniscalco@chromium.org. > > The reason for reverting is: Speculatively reverting. Suspect it may be cause > of leaks on linux asan lsan bot: > > http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... > . This is also the likely the cause of the tsan bot on MFYI suddenly reporting dozens of races: the tool seems to fail to get file and line information, and its suppressions are based on filenames, so all of its suppressions fail. See http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28T...
Message was sent while issue was closed.
On 2014/10/09 23:35:02, Derek Bruening wrote: > On 2014/10/09 22:45:52, maniscalco wrote: > > A revert of this CL (patchset #1 id:1) has been created in > > https://codereview.chromium.org/649433002/ by mailto:maniscalco@chromium.org. > > > > The reason for reverting is: Speculatively reverting. Suspect it may be cause > > of leaks on linux asan lsan bot: > > > > > http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... > > . > > This is also the likely the cause of the tsan bot on MFYI suddenly reporting > dozens of races: the tool seems to fail to get file and line information, and > its suppressions are based on filenames, so all of its suppressions fail. See > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28T... Any way to verify it is this change causes the problem?
Message was sent while issue was closed.
> Any way to verify it is this change causes the problem? The bot went green as soon as the revert went in, so it does seem likely that this caused the problem. First green build: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... Not sure why this would cause such a problem, though.
Message was sent while issue was closed.
On 2014/10/10 17:11:19, brettw wrote: > > Any way to verify it is this change causes the problem? > > The bot went green as soon as the revert went in, so it does seem likely that > this caused the problem. First green build: > http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... > > Not sure why this would cause such a problem, though. How can I test this bot with this patch again? Thanks |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
