Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
659543003:
Fix LSan on swarming. (Closed)
DescriptionFix LSan on swarming.
ASAN_OPTIONS=detect_leaks=1 needs to be passed in.
BUG=414808
R=earthdok@chromium.org, maruel@chromium.org
Committed: https://crrev.com/14ae054faefb81d8a1f8de1c3e0f399902d65843
Cr-Commit-Position: refs/heads/master@{#299712}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : fixes #Patch Set 4 : fix browser tests #Patch Set 5 : cleanup #
Total comments: 30
Patch Set 6 : updates #
Total comments: 2
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #Messages
Total messages: 40 (6 generated)
jam@chromium.org changed reviewers: + earthdok@chromium.org
maruel@chromium.org changed reviewers: + maruel@chromium.org
Generally lgtm, a few style comments, no comment on the actual behavior, as I'm not familiar with it. https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:87: if asan: I think it's large enough I'd make it a function accepting (env, asan, lsan) for general readability. https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:100: asan_options += ['detect_leaks=1'] I really prefer: asan_options.append('detect_leaks=1') https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:104: env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' [General comment] It'd be nice to have these additive instead of overriding the environment variable. In practice I don't think it's going to be a problem though. https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:111: lsan_options = ['suppressions=../tools/lsan/suppressions.txt', The relative path could be wrong; it's relative to the .gyp target. It's just by luck that they are all exactly one directory below src and not two. It'd be preferable to make it an absolute path, e.g. os.path.join(os.path.dirname(ROOT_DIR), 'tools/lsan/suppressions.txt')
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #4 (id:100001) has been deleted
Thanks Marc-Antoine for the review, please take another look. Sergey: thanks for catching this and for the tips about which flags are needed now with the base changes. Here are runs that I triggered from my local Linux box for all the swarmed test suites on the LSan builder: https://chromium-swarm.appspot.com/user/task/14911c7efce8700 https://chromium-swarm.appspot.com/user/task/14911c78ee58400 https://chromium-swarm.appspot.com/user/task/14911c82ca79600 https://chromium-swarm.appspot.com/user/task/14911c861b3cf00 https://chromium-swarm.appspot.com/user/task/14911c8a145b600 https://chromium-swarm.appspot.com/user/task/14911c8db745400 https://chromium-swarm.appspot.com/user/task/14911c9069faa00 https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:87: if asan: On 2014/10/14 23:38:51, M-A Ruel wrote: > I think it's large enough I'd make it a function accepting (env, asan, lsan) for > general readability. Done. https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:100: asan_options += ['detect_leaks=1'] On 2014/10/14 23:38:51, M-A Ruel wrote: > I really prefer: > asan_options.append('detect_leaks=1') Done. https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:104: env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' On 2014/10/14 23:38:51, M-A Ruel wrote: > [General comment] It'd be nice to have these additive instead of overriding the > environment variable. In practice I don't think it's going to be a problem > though. Done. https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... testing/test_env.py:111: lsan_options = ['suppressions=../tools/lsan/suppressions.txt', On 2014/10/14 23:38:51, M-A Ruel wrote: > The relative path could be wrong; it's relative to the .gyp target. It's just by > luck that they are all exactly one directory below src and not two. > > It'd be preferable to make it an absolute path, e.g. > os.path.join(os.path.dirname(ROOT_DIR), 'tools/lsan/suppressions.txt') > Done. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:63: cmd.remove(i) Marc-Antoine: I added this method to remove the extra flags that are used to communicate with test_env.py. I figured there's no reason these internal flags should be exposed to test binaries, and this makes swarmed tests more similar to running them locally. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' Sergey: I don't know why I get these leaks with G_SLICE, even though they were passed before swarming. Even running locally I get them. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:156: ' Command: %s\n' % (str(extra_env), str(cmd)) Marc-Antoine: Sergey brought up a good point over email, which is that we lost this information after switching to swarming. i.e. before run_tests would show the environment variables and the command. So I started printing it here. This would have helped catch this regression earlier because it would be easy to see how the test is run.
On 2014/10/15 03:20:48, jam wrote: > Thanks Marc-Antoine for the review, please take another look. > > Sergey: thanks for catching this and for the tips about which flags are needed > now with the base changes. > > Here are runs that I triggered from my local Linux box for all the swarmed test > suites on the LSan builder: > https://chromium-swarm.appspot.com/user/task/14911c7efce8700 > https://chromium-swarm.appspot.com/user/task/14911c78ee58400 btw this one is browser_tests. it said all tests passed at the end, but the task is listed as a Failure with Exit codes [-9, 0]. Any idea why? i.e. is it because I didn't split it up and I ran it in one shard only? Either way, the UI is a bit confusing as is. > https://chromium-swarm.appspot.com/user/task/14911c82ca79600 > https://chromium-swarm.appspot.com/user/task/14911c861b3cf00 > https://chromium-swarm.appspot.com/user/task/14911c8a145b600 > https://chromium-swarm.appspot.com/user/task/14911c8db745400 > https://chromium-swarm.appspot.com/user/task/14911c9069faa00 > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py > File testing/test_env.py (right): > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > testing/test_env.py:87: if asan: > On 2014/10/14 23:38:51, M-A Ruel wrote: > > I think it's large enough I'd make it a function accepting (env, asan, lsan) > for > > general readability. > > Done. > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > testing/test_env.py:100: asan_options += ['detect_leaks=1'] > On 2014/10/14 23:38:51, M-A Ruel wrote: > > I really prefer: > > asan_options.append('detect_leaks=1') > > Done. > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > testing/test_env.py:104: env['LD_LIBRARY_PATH'] = > '/usr/lib/x86_64-linux-gnu/debug:' > On 2014/10/14 23:38:51, M-A Ruel wrote: > > [General comment] It'd be nice to have these additive instead of overriding > the > > environment variable. In practice I don't think it's going to be a problem > > though. > > Done. > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > testing/test_env.py:111: lsan_options = > ['suppressions=../tools/lsan/suppressions.txt', > On 2014/10/14 23:38:51, M-A Ruel wrote: > > The relative path could be wrong; it's relative to the .gyp target. It's just > by > > luck that they are all exactly one directory below src and not two. > > > > It'd be preferable to make it an absolute path, e.g. > > os.path.join(os.path.dirname(ROOT_DIR), 'tools/lsan/suppressions.txt') > > > > Done. > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py > File testing/test_env.py (right): > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:63: cmd.remove(i) > Marc-Antoine: I added this method to remove the extra flags that are used to > communicate with test_env.py. I figured there's no reason these internal flags > should be exposed to test binaries, and this makes swarmed tests more similar to > running them locally. > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > passed before swarming. Even running locally I get them. > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:156: ' Command: %s\n' % (str(extra_env), str(cmd)) > Marc-Antoine: Sergey brought up a good point over email, which is that we lost > this information after switching to swarming. i.e. before run_tests would show > the environment variables and the command. So I started printing it here. > > This would have helped catch this regression earlier because it would be easy to > see how the test is run.
On 2014/10/15 05:27:27, jam wrote: > On 2014/10/15 03:20:48, jam wrote: > > Thanks Marc-Antoine for the review, please take another look. > > > > Sergey: thanks for catching this and for the tips about which flags are needed > > now with the base changes. > > > > Here are runs that I triggered from my local Linux box for all the swarmed > test > > suites on the LSan builder: > > https://chromium-swarm.appspot.com/user/task/14911c7efce8700 > > https://chromium-swarm.appspot.com/user/task/14911c78ee58400 > > btw this one is browser_tests. it said all tests passed at the end, but the task > is listed as a Failure with Exit codes [-9, 0]. Any idea why? i.e. is it because > I didn't split it up and I ran it in one shard only? Either way, the UI is a bit > confusing as is. I tried this out with 5 shards and they succeeded: https://chromium-swarm.appspot.com/user/task/149124f9e5c3b00 https://chromium-swarm.appspot.com/user/task/149124fa002dd00 https://chromium-swarm.appspot.com/user/task/149124fa1a98600 https://chromium-swarm.appspot.com/user/task/149124fa420ad00 https://chromium-swarm.appspot.com/user/task/149124faa714500 > > > https://chromium-swarm.appspot.com/user/task/14911c82ca79600 > > https://chromium-swarm.appspot.com/user/task/14911c861b3cf00 > > https://chromium-swarm.appspot.com/user/task/14911c8a145b600 > > https://chromium-swarm.appspot.com/user/task/14911c8db745400 > > https://chromium-swarm.appspot.com/user/task/14911c9069faa00 > > > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py > > File testing/test_env.py (right): > > > > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > > testing/test_env.py:87: if asan: > > On 2014/10/14 23:38:51, M-A Ruel wrote: > > > I think it's large enough I'd make it a function accepting (env, asan, lsan) > > for > > > general readability. > > > > Done. > > > > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > > testing/test_env.py:100: asan_options += ['detect_leaks=1'] > > On 2014/10/14 23:38:51, M-A Ruel wrote: > > > I really prefer: > > > asan_options.append('detect_leaks=1') > > > > Done. > > > > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > > testing/test_env.py:104: env['LD_LIBRARY_PATH'] = > > '/usr/lib/x86_64-linux-gnu/debug:' > > On 2014/10/14 23:38:51, M-A Ruel wrote: > > > [General comment] It'd be nice to have these additive instead of overriding > > the > > > environment variable. In practice I don't think it's going to be a problem > > > though. > > > > Done. > > > > > https://codereview.chromium.org/659543003/diff/20001/testing/test_env.py#newc... > > testing/test_env.py:111: lsan_options = > > ['suppressions=../tools/lsan/suppressions.txt', > > On 2014/10/14 23:38:51, M-A Ruel wrote: > > > The relative path could be wrong; it's relative to the .gyp target. It's > just > > by > > > luck that they are all exactly one directory below src and not two. > > > > > > It'd be preferable to make it an absolute path, e.g. > > > os.path.join(os.path.dirname(ROOT_DIR), 'tools/lsan/suppressions.txt') > > > > > > > Done. > > > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py > > File testing/test_env.py (right): > > > > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > > testing/test_env.py:63: cmd.remove(i) > > Marc-Antoine: I added this method to remove the extra flags that are used to > > communicate with test_env.py. I figured there's no reason these internal flags > > should be exposed to test binaries, and this makes swarmed tests more similar > to > > running them locally. > > > > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > > testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' > > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > > passed before swarming. Even running locally I get them. > > > > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > > testing/test_env.py:156: ' Command: %s\n' % (str(extra_env), str(cmd)) > > Marc-Antoine: Sergey brought up a good point over email, which is that we lost > > this information after switching to swarming. i.e. before run_tests would show > > the environment variables and the command. So I started printing it here. > > > > This would have helped catch this regression earlier because it would be easy > to > > see how the test is run.
purely styling nits https://codereview.chromium.org/659543003/diff/120001/base/base.isolate File base/base.isolate (right): https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... base/base.isolate:31: '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', I assume we don't use asan on ARM so we're fine. We can see in time if this ever happens. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:60: internal_commands = ['--asan=0', '--asan=1', '--lsan=0', '--lsan=1'] internal_commands = frozenset(['--asan=0', '--asan=1', '--lsan=0', '--lsan=1']) return [i for i in cmd if i not in internal_commands] Then update line 149 to be: cmd = trim_cmd(cmd) I prefer this to mutating variables in place, it's much harder to follow. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:76: def setup_asan(cmd, extra_env, lsan): I also prefer it to create a extra_env locally and return it, then caller can env.update(setup_asan(cmd, lsan)) But that doesn't work when the code wants to append something to an existing variable but that's already the flow you use at line 157, so I'd prefer that. I'm not a fan of in-out parameters in python and prefer to have out arguments to be returned. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:77: # Instruct GTK to use malloc while running ASan or LSan tests. """Instructs GTK ... " https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:126: two lines https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:156: ' Command: %s\n' % (str(extra_env), str(cmd)) Ok, I'd prefer something like: print('test_env.py enforced env vars: %s' % ' '.join('%s=%s' % (k, v) for k, v in sorted(extra_env.iteritems()))) print(' Command: %s' % ' '.join(cmd))
https://codereview.chromium.org/659543003/diff/120001/base/base.isolate File base/base.isolate (right): https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... base/base.isolate:31: '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', I don't think you need this dependency. Please see the comment in test_env.py. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:54: extra_env['CHROME_DEVEL_SANDBOX'] = '' This is not good. Please pass --no-sandbox, like we do in runtest.py. Unsetting CHROME_DEVEL_SANDBOX is a deprecated (see http://crbug.com/245376) equivalent of --disable-setuid-sandbox, which is weaker than --no-sandbox. (Whether --disable-setuid-sandbox would be sufficient for LSan is an interesting question though.) https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' On 2014/10/15 03:20:48, jam wrote: > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > passed before swarming. Even running locally I get them. I'll take a look. It's possible that someone introduced a GTK leak in the last two weeks. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:98: extra_env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' So this is probably useless to you. This points to a system-installed debug version of libstdc++, which requires the libstdc++-dbg package. Swarming slaves likely won't have it installed, so this does nothing. Note that this does not point to third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6 (the added dependency in base.isolate). It is news to me that we have a copy of libstdc++ under llvm-build. We could use it here instead of the system-installed one. However, we're switching ASan builds to just-built libc++, which means no mode dependency on libstdc++, which means we can drop this variable altogether. Linux ASan builds have already switched, but Linux ChromeOS ASan still uses libstdc++. We can probably live with incomplete stacks on the ChromeOS bots for a short while.
On 2014/10/15 14:11:46, earthdok wrote: > https://codereview.chromium.org/659543003/diff/120001/base/base.isolate > File base/base.isolate (right): > > https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... > base/base.isolate:31: > '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', > I don't think you need this dependency. Please see the comment in test_env.py. > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py > File testing/test_env.py (right): > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:54: extra_env['CHROME_DEVEL_SANDBOX'] = '' > This is not good. Please pass --no-sandbox, like we do in runtest.py. > > Unsetting CHROME_DEVEL_SANDBOX is a deprecated (see http://crbug.com/245376) > equivalent of --disable-setuid-sandbox, which is weaker than --no-sandbox. > > (Whether --disable-setuid-sandbox would be sufficient for LSan is an interesting > question though.) > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' > On 2014/10/15 03:20:48, jam wrote: > > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > > passed before swarming. Even running locally I get them. > > I'll take a look. It's possible that someone introduced a GTK leak in the last > two weeks. > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:98: extra_env['LD_LIBRARY_PATH'] = > '/usr/lib/x86_64-linux-gnu/debug:' > So this is probably useless to you. This points to a system-installed debug > version of libstdc++, which requires the libstdc++-dbg package. Swarming slaves > likely won't have it installed, so this does nothing. > > Note that this does not point to > third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6 (the added dependency > in base.isolate). It is news to me that we have a copy of libstdc++ under > llvm-build. We could use it here instead of the system-installed one. However, > we're switching ASan builds to just-built libc++, which means no mode dependency "no more dependency" > on libstdc++, which means we can drop this variable altogether. > > Linux ASan builds have already switched, but Linux ChromeOS ASan still uses > libstdc++. We can probably live with incomplete stacks on the ChromeOS bots for > a short while.
https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:98: extra_env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' On 2014/10/15 14:11:45, earthdok wrote: > Linux ASan builds have already switched, but Linux ChromeOS ASan still uses > libstdc++. We can probably live with incomplete stacks on the ChromeOS bots for > a short while. On second thought, let's change this to point to third_party/llvm-build/Release+Asserts/lib/ for now. I'm not sure how long fixing the ChromeOS issue will take.
ptal https://codereview.chromium.org/659543003/diff/120001/base/base.isolate File base/base.isolate (right): https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... base/base.isolate:31: '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', On 2014/10/15 14:11:45, earthdok wrote: > I don't think you need this dependency. Please see the comment in test_env.py. replied there https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... base/base.isolate:31: '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', On 2014/10/15 13:07:59, M-A Ruel wrote: > I assume we don't use asan on ARM so we're fine. We can see in time if this ever > happens. right https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:54: extra_env['CHROME_DEVEL_SANDBOX'] = '' On 2014/10/15 14:11:45, earthdok wrote: > This is not good. Please pass --no-sandbox, like we do in runtest.py. > > Unsetting CHROME_DEVEL_SANDBOX is a deprecated (see http://crbug.com/245376) > equivalent of --disable-setuid-sandbox, which is weaker than --no-sandbox. run_test.py also unsets CHROME_DEVEL_SANDBOX. so it doesn't need to do both? either way, i'm happy to do this but not in this cl. i'd like to keep it focused on fixing lsan without changing other stuff like this to reduce the risk that it breaks something else. > > (Whether --disable-setuid-sandbox would be sufficient for LSan is an interesting > question though.) https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:60: internal_commands = ['--asan=0', '--asan=1', '--lsan=0', '--lsan=1'] On 2014/10/15 13:08:00, M-A Ruel wrote: > internal_commands = frozenset(['--asan=0', '--asan=1', '--lsan=0', '--lsan=1']) > return [i for i in cmd if i not in internal_commands] > > Then update line 149 to be: > cmd = trim_cmd(cmd) > > I prefer this to mutating variables in place, it's much harder to follow. Done. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:76: def setup_asan(cmd, extra_env, lsan): On 2014/10/15 13:07:59, M-A Ruel wrote: > I also prefer it to create a extra_env locally and return it, then caller can > env.update(setup_asan(cmd, lsan)) > > But that doesn't work when the code wants to append something to an existing > variable but that's already the flow you use at line 157, so I'd prefer that. > I'm not a fan of in-out parameters in python and prefer to have out arguments to > be returned. Done. also enable_sandbox_if_required https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:77: # Instruct GTK to use malloc while running ASan or LSan tests. On 2014/10/15 13:07:59, M-A Ruel wrote: > """Instructs GTK ... " that would be for the function comment right? this comment is for the line below. I added a function comment https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:98: extra_env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' On 2014/10/15 14:35:56, earthdok wrote: > On 2014/10/15 14:11:45, earthdok wrote: > > Linux ASan builds have already switched, but Linux ChromeOS ASan still uses > > libstdc++. We can probably live with incomplete stacks on the ChromeOS bots > for > > a short while. > > On second thought, let's change this to point to > third_party/llvm-build/Release+Asserts/lib/ for now. I'm not sure how long > fixing the ChromeOS issue will take. I just checked and the swarm slaves, based on connecting to a few of them (and given that they're the same image), have /usr/lib/x86_64-linux-gnu/debug since this is what run_test.py does, it seems fine to leave as is. then when run_test.py is updated, this script can be updated as well. somewhat of a sidetrack: it's been hard to figure out what I need in this script given how much of the stuff in run_test.py isn't needed. can someone remove all the unnecessary setting of flags in it? https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:126: On 2014/10/15 13:08:00, M-A Ruel wrote: > two lines Done. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:156: ' Command: %s\n' % (str(extra_env), str(cmd)) On 2014/10/15 13:08:00, M-A Ruel wrote: > Ok, I'd prefer something like: > > print('test_env.py enforced env vars: %s' % > ' '.join('%s=%s' % (k, v) for k, v in sorted(extra_env.iteritems()))) > print(' Command: %s' % ' '.join(cmd)) Done.
https://codereview.chromium.org/659543003/diff/140001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/140001/testing/test_env.py#new... testing/test_env.py:163: print 'Additional test environment:\n%s\n' \ Use () instead of \
https://codereview.chromium.org/659543003/diff/140001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/140001/testing/test_env.py#new... testing/test_env.py:163: print 'Additional test environment:\n%s\n' \ On 2014/10/15 15:27:09, M-A Ruel wrote: > Use () instead of \ Done.
lgtm https://codereview.chromium.org/659543003/diff/160001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/160001/testing/test_env.py#new... testing/test_env.py:163: print ('Additional test environment:\n%s\n' print is now a function with python 2.5 (?) so you can do: print(... without a space. Statement print is only kept as compatibility.
https://codereview.chromium.org/659543003/diff/160001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/160001/testing/test_env.py#new... testing/test_env.py:163: print ('Additional test environment:\n%s\n' On 2014/10/15 16:14:34, M-A Ruel wrote: > print is now a function with python 2.5 (?) so you can do: > print(... without a space. > Statement print is only kept as compatibility. Done.
https://codereview.chromium.org/659543003/diff/120001/base/base.isolate File base/base.isolate (right): https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... base/base.isolate:31: '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', On 2014/10/15 15:23:25, jam wrote: > On 2014/10/15 14:11:45, earthdok wrote: > > I don't think you need this dependency. Please see the comment in test_env.py. > > replied there Maybe I'm misunderstanding. If (according to your reply) you want to use /usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6 then what's this dependency for? Is it for llvm-symbolizer? https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:54: extra_env['CHROME_DEVEL_SANDBOX'] = '' On 2014/10/15 15:23:25, jam wrote: > On 2014/10/15 14:11:45, earthdok wrote: > > This is not good. Please pass --no-sandbox, like we do in runtest.py. > > > > Unsetting CHROME_DEVEL_SANDBOX is a deprecated (see http://crbug.com/245376) > > equivalent of --disable-setuid-sandbox, which is weaker than --no-sandbox. > > run_test.py also unsets CHROME_DEVEL_SANDBOX. so it doesn't need to do both? Looking at this conditional in runtest.py, there are two scenarios: https://code.google.com/p/chromium/codesearch#chromium/tools/build/scripts/sl... - LSan or TSan is enabled, in which case we append --no-sandbox later on, and unsetting CHROME_DEVEL_SANDBOX here is superfluous, - neither LSan or TSan is enabled, but _ShouldEnableSandbox returns false. In this case we don't append --no-sandbox; we rely exclusively on unsetting the env variable to disable the setuid sandbox. This configuration is not used by sanitizers. I don't know which builders (if any) use it. > either way, i'm happy to do this but not in this cl. i'd like to keep it focused > on fixing lsan without changing other stuff like this to reduce the risk that it > breaks something else. I do not insist that we get rid of this env variable as part of this CL. Rather, I propose to append "--no-sandbox" in LSan runs. Not doing so is a regression from runtest.py, and arguably should be fixed as part of fixing lsan. However, since your test runs weren't significantly broken by this regression, I'm ok with doing this in a follow-up CL. > > > > (Whether --disable-setuid-sandbox would be sufficient for LSan is an > interesting > > question though.) > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:98: extra_env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' On 2014/10/15 15:23:25, jam wrote: > On 2014/10/15 14:35:56, earthdok wrote: > > On 2014/10/15 14:11:45, earthdok wrote: > > > Linux ASan builds have already switched, but Linux ChromeOS ASan still uses > > > libstdc++. We can probably live with incomplete stacks on the ChromeOS bots > > for > > > a short while. > > > > On second thought, let's change this to point to > > third_party/llvm-build/Release+Asserts/lib/ for now. I'm not sure how long > > fixing the ChromeOS issue will take. > > I just checked and the swarm slaves, based on connecting to a few of them (and > given that they're the same image), have /usr/lib/x86_64-linux-gnu/debug But does that directory contain libstc++ DSOs? > since this is what run_test.py does, it seems fine to leave as is. then when > run_test.py is updated, this script can be updated as well. > > somewhat of a sidetrack: it's been hard to figure out what I need in this script > given how much of the stuff in run_test.py isn't needed. can someone remove all > the unnecessary setting of flags in it? Yeah, clean-up is long overdue. We also need to move more stuff to default options. I'll take a look.
https://codereview.chromium.org/659543003/diff/120001/base/base.isolate File base/base.isolate (right): https://codereview.chromium.org/659543003/diff/120001/base/base.isolate#newco... base/base.isolate:31: '../third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6', On 2014/10/15 17:00:37, earthdok wrote: > On 2014/10/15 15:23:25, jam wrote: > > On 2014/10/15 14:11:45, earthdok wrote: > > > I don't think you need this dependency. Please see the comment in > test_env.py. > > > > replied there > > Maybe I'm misunderstanding. If (according to your reply) you want to use > /usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6 then what's this dependency for? > Is it for llvm-symbolizer? sorry my earlier reply was wrong. yes this is needed for symbolizer. without it I didn't get stack traces. https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:54: extra_env['CHROME_DEVEL_SANDBOX'] = '' On 2014/10/15 17:00:37, earthdok wrote: > On 2014/10/15 15:23:25, jam wrote: > > On 2014/10/15 14:11:45, earthdok wrote: > > > This is not good. Please pass --no-sandbox, like we do in runtest.py. > > > > > > Unsetting CHROME_DEVEL_SANDBOX is a deprecated (see http://crbug.com/245376) > > > equivalent of --disable-setuid-sandbox, which is weaker than --no-sandbox. > > > > run_test.py also unsets CHROME_DEVEL_SANDBOX. so it doesn't need to do both? > > Looking at this conditional in runtest.py, there are two scenarios: > https://code.google.com/p/chromium/codesearch#chromium/tools/build/scripts/sl... > > - LSan or TSan is enabled, in which case we append --no-sandbox later on, and > unsetting CHROME_DEVEL_SANDBOX here is superfluous, > - neither LSan or TSan is enabled, but _ShouldEnableSandbox returns false. In > this case we don't append --no-sandbox; we rely exclusively on unsetting the env > variable to disable the setuid sandbox. This configuration is not used by > sanitizers. I don't know which builders (if any) use it. > > > either way, i'm happy to do this but not in this cl. i'd like to keep it > focused > > on fixing lsan without changing other stuff like this to reduce the risk that > it > > breaks something else. > > I do not insist that we get rid of this env variable as part of this CL. Rather, > I propose to append "--no-sandbox" in LSan runs. Not doing so is a regression > from runtest.py, and arguably should be fixed as part of fixing lsan. good point. I added it. > > However, since your test runs weren't significantly broken by this regression, > I'm ok with doing this in a follow-up CL. > > > > > > > (Whether --disable-setuid-sandbox would be sufficient for LSan is an > > interesting > > > question though.) > > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:98: extra_env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:' On 2014/10/15 17:00:37, earthdok wrote: > On 2014/10/15 15:23:25, jam wrote: > > On 2014/10/15 14:35:56, earthdok wrote: > > > On 2014/10/15 14:11:45, earthdok wrote: > > > > Linux ASan builds have already switched, but Linux ChromeOS ASan still > uses > > > > libstdc++. We can probably live with incomplete stacks on the ChromeOS > bots > > > for > > > > a short while. > > > > > > On second thought, let's change this to point to > > > third_party/llvm-build/Release+Asserts/lib/ for now. I'm not sure how long > > > fixing the ChromeOS issue will take. > > > > I just checked and the swarm slaves, based on connecting to a few of them (and > > given that they're the same image), have /usr/lib/x86_64-linux-gnu/debug > But does that directory contain libstc++ DSOs? yep chrome-bot@swarm126-c4:~$ ls /usr/lib/x86_64-linux-gnu/debug/libstdc++* /usr/lib/x86_64-linux-gnu/debug/libstdc++.a /usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6@ /usr/lib/x86_64-linux-gnu/debug/libstdc++.so@ /usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6.0.16 > > > since this is what run_test.py does, it seems fine to leave as is. then when > > run_test.py is updated, this script can be updated as well. > > > > somewhat of a sidetrack: it's been hard to figure out what I need in this > script > > given how much of the stuff in run_test.py isn't needed. can someone remove > all > > the unnecessary setting of flags in it? > > Yeah, clean-up is long overdue. We also need to move more stuff to default > options. I'll take a look.
lgtm
btw here are the swarming runs for the last patchset (9) https://chromium-swarm.appspot.com/user/task/14914e04d66e400 https://chromium-swarm.appspot.com/user/task/14914e066fed900 https://chromium-swarm.appspot.com/user/task/14914e07f632200 https://chromium-swarm.appspot.com/user/task/14914e08e20b500 https://chromium-swarm.appspot.com/user/task/14914e09df0fe00 https://chromium-swarm.appspot.com/user/task/14914e0ac0bcf00 https://chromium-swarm.appspot.com/user/task/14914e117bff000 https://chromium-swarm.appspot.com/user/task/14914e11a91fe00 https://chromium-swarm.appspot.com/user/task/14914e11e0a7b00 https://chromium-swarm.appspot.com/user/task/14914e124276000 https://chromium-swarm.appspot.com/user/task/14914e12568f500
On 2014/10/15 17:37:25, jam wrote: > btw here are the swarming runs for the last patchset (9) > > https://chromium-swarm.appspot.com/user/task/14914e04d66e400 > https://chromium-swarm.appspot.com/user/task/14914e066fed900 > https://chromium-swarm.appspot.com/user/task/14914e07f632200 > https://chromium-swarm.appspot.com/user/task/14914e08e20b500 > https://chromium-swarm.appspot.com/user/task/14914e09df0fe00 > https://chromium-swarm.appspot.com/user/task/14914e0ac0bcf00 > https://chromium-swarm.appspot.com/user/task/14914e117bff000 > https://chromium-swarm.appspot.com/user/task/14914e11a91fe00 > https://chromium-swarm.appspot.com/user/task/14914e11e0a7b00 > https://chromium-swarm.appspot.com/user/task/14914e124276000 > https://chromium-swarm.appspot.com/user/task/14914e12568f500 The environment looks right. It's weird that there are no failures. I'd expect at least a few leaks to seep in over two weeks.
Message was sent while issue was closed.
Committed patchset #9 (id:200001) manually as 14ae054faefb81d8a1f8de1c3e0f399902d65843.
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/14ae054faefb81d8a1f8de1c3e0f399902d65843 Cr-Commit-Position: refs/heads/master@{#299712}
Message was sent while issue was closed.
https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py File testing/test_env.py (right): https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' On 2014/10/15 03:20:48, jam wrote: > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > passed before swarming. Even running locally I get them. Could you please link to a test run where they reproduced?
Message was sent while issue was closed.
On 2014/10/15 19:57:35, earthdok wrote: > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py > File testing/test_env.py (right): > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' > On 2014/10/15 03:20:48, jam wrote: > > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > > passed before swarming. Even running locally I get them. > > Could you please link to a test run where they reproduced? This change seems to have broken the NaCl SDK builders (and presumably other browser_tester tests, if any): http://build.chromium.org/p/client.nacl.sdk/builders/mac-sdk-multi/builds/923... ppapi/native_client/tools/browser_tester/browser_tester.py is relying on test_env having the function enable_sandbox_if_required.
Message was sent while issue was closed.
On 2014/10/15 19:57:35, earthdok wrote: > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py > File testing/test_env.py (right): > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' > On 2014/10/15 03:20:48, jam wrote: > > Sergey: I don't know why I get these leaks with G_SLICE, even though they were > > passed before swarming. Even running locally I get them. > > Could you please link to a test run where they reproduced? This was just running locally
Message was sent while issue was closed.
On 2014/10/15 21:10:35, binji wrote: > On 2014/10/15 19:57:35, earthdok wrote: > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py > > File testing/test_env.py (right): > > > > > https://codereview.chromium.org/659543003/diff/120001/testing/test_env.py#new... > > testing/test_env.py:83: #extra_env['G_SLICE'] = 'always-malloc' > > On 2014/10/15 03:20:48, jam wrote: > > > Sergey: I don't know why I get these leaks with G_SLICE, even though they > were > > > passed before swarming. Even running locally I get them. > > > > Could you please link to a test run where they reproduced? > > This change seems to have broken the NaCl SDK builders (and presumably other > browser_tester tests, if any): > > http://build.chromium.org/p/client.nacl.sdk/builders/mac-sdk-multi/builds/923... > > ppapi/native_client/tools/browser_tester/browser_tester.py is relying on > test_env having the function enable_sandbox_if_required. yeah sorry about that, I didn't realize that script was used elsewhere. I've committed a fix.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This looks pretty different from the asan options on runtest.py: https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ru... Is that intentional? Is there no better way than cloning parts of runtest.py here? Is the plan to stop using runtest.py eventually?
Message was sent while issue was closed.
On 2015/01/28 06:12:33, Nico wrote: > This looks pretty different from the asan options on runtest.py: > https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ru... The ASan options from runtest.py have been superseded by build/sanitizers/sanitizer_options.cc. It should be safe to remove them. > Is that intentional? > > Is there no better way than cloning parts of runtest.py here? Is the plan to > stop using runtest.py eventually?
Message was sent while issue was closed.
On 2015/01/28 08:53:25, Alexander Potapenko wrote: > On 2015/01/28 06:12:33, Nico wrote: > > This looks pretty different from the asan options on runtest.py: > > > https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ru... > The ASan options from runtest.py have been superseded by > build/sanitizers/sanitizer_options.cc. It should be safe to remove them. Well, not all of them. Some depend on the bot configuration and can't be set in the binary. I don't know whether runtest.py is going to be used after the switch to swarming (my guess is that it is not).
Message was sent while issue was closed.
On Wed, Jan 28, 2015 at 1:00 AM, <glider@chromium.org> wrote: > On 2015/01/28 08:53:25, Alexander Potapenko wrote: > >> On 2015/01/28 06:12:33, Nico wrote: >> > This looks pretty different from the asan options on runtest.py: >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > build/scripts/slave/runtest.py&l=2032 > >> The ASan options from runtest.py have been superseded by >> build/sanitizers/sanitizer_options.cc. It should be safe to remove them. >> > > Well, not all of them. Some depend on the bot configuration and can't be > set in > the binary. > I don't know whether runtest.py is going to be used after the switch to > swarming > (my guess is that it is not). > From what I understand, swarming uses the command lines in .isolate files, and these bake testing/test_env.py into the packaged file – so test_env.py needs to behave like runtest.py does (hence, this CL for example). Maybe you can compare the asan/lsan/etc stuff in test_env.py and runtest.py and make sure that the differences are all ok? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
That's right. Note that it's not a requirement to bake *everything* in test_env.py. I'd be more than happy to have an test_env_asan.py or something like that, maintainability wise. M-A 2015-01-28 6:16 GMT-08:00 Nico Weber <thakis@chromium.org>: > On Wed, Jan 28, 2015 at 1:00 AM, <glider@chromium.org> wrote: > >> On 2015/01/28 08:53:25, Alexander Potapenko wrote: >> >>> On 2015/01/28 06:12:33, Nico wrote: >>> > This looks pretty different from the asan options on runtest.py: >>> > >>> >> >> https://code.google.com/p/chromium/codesearch#chromium/ >> build/scripts/slave/runtest.py&l=2032 >> >>> The ASan options from runtest.py have been superseded by >>> build/sanitizers/sanitizer_options.cc. It should be safe to remove them. >>> >> >> Well, not all of them. Some depend on the bot configuration and can't be >> set in >> the binary. >> I don't know whether runtest.py is going to be used after the switch to >> swarming >> (my guess is that it is not). >> > > From what I understand, swarming uses the command lines in .isolate files, > and these bake testing/test_env.py into the packaged file – so test_env.py > needs to behave like runtest.py does (hence, this CL for example). Maybe > you can compare the asan/lsan/etc stuff in test_env.py and runtest.py and > make sure that the differences are all ok? > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/01/28 15:10:33, M-A Ruel wrote: > That's right. > > Note that it's not a requirement to bake *everything* in test_env.py. I'd > be more than happy to have an test_env_asan.py or something like that, > maintainability wise. > > M-A It would be pretty awesome if we could hoist all sanitizer-related stuff out of runtest.py and put it somewhere it could be shared between runtest.py and test_env.py. Where would be a good place for it though? If we put it under src/testing/, that would make runtest.py dependent on a Chromium checkout. Some of the users of that script don't have a Chromium checkout (GPU trybots come to mind). Granted, it's likely that none of them use ASan, as there's already a dependency on llvm-symbolizer in that case. This means we could have something like: if options.enable_asan or options.enable_msan or ...: import test_env_asan But I'm not super psyched about this solution.
Message was sent while issue was closed.
To correct myself, not all platforms are currently supported by sanitizer_options.cc (namely CrOS is not), so we can't blindly move options from runtest.py to sanitizer_options.cc I agree test_env.py should better not duplicate runtest.py However I don't think we can't make runtest.py (which is in build/) depend on something from src/. Likewise, test_env.py must not depend on build/.
Message was sent while issue was closed.
On 2015/01/28 15:30:20, earthdok wrote: > On 2015/01/28 15:10:33, M-A Ruel wrote: > > That's right. > > > > Note that it's not a requirement to bake *everything* in test_env.py. I'd > > be more than happy to have an test_env_asan.py or something like that, > > maintainability wise. > > > > M-A > > It would be pretty awesome if we could hoist all sanitizer-related stuff out of > runtest.py and put it somewhere it could be shared between runtest.py and > test_env.py. > > Where would be a good place for it though? If we put it under src/testing/, that > would make runtest.py dependent on a Chromium checkout. Some of the users of > that script don't have a Chromium checkout (GPU trybots come to mind). Granted, > it's likely that none of them use ASan, as there's already a dependency on > llvm-symbolizer in that case. This means we could have something like: > > if options.enable_asan or options.enable_msan or ...: > import test_env_asan > > But I'm not super psyched about this solution. Here's an idea: runtest.py could have a --env_setup_script parameter that it'd run before running tests, and then the chromium bot config could pass in --env_setup_script=src/testing/test_env.py . Then runtest.py wouldn't depend on src directly, and the bot configs already know about both build and src. |
