Closed Bug 1587463 Opened 3 months ago Closed 3 months ago

Consider using builtin suppression lists for LSan and TSan

Categories

(Firefox Build System :: General, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

(firefox71 wontfix, firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

Historically, we have been using suppression lists in build/sanitizer/ for ignoring errors with LSan and TSan. This makes working with both tools complex, because the suppression files have to be copied for every test suite type (Mochitest, WPT and xpcshell tests) and for fuzzing, as well as configured through environment variables.

For a while now, sanitizers are supporting builtin default lists by linking a file containing functions like extern "C" const char *__lsan_default_suppressions() and extern "C" const char *__tsan_default_suppressions() respectively, returning the lists in the same format as the current text files. This is similar to the weak default_options hook that we already use for ASan.

I believe this would make using the tools significantly easier, as it would "just work" in every automation without carrying around additional files and configuring their usage in the environment.

The lists are also additive, so if you specify another suppressions file through the runtime options, they will be merged.

Duplicate of this bug: 1587065

These two patches move our suppression lists for LSan, TSan and UBSan (though UBSan's list is currently empty) into the build and simplify our automation code respectively. I pushed this to try with a subset of all tests and it is green for ASan (so LSan suppressions must be working). For TSan I also get a green build (tests are orange due to remaining races, but we neither have a TSan build nor tests running right now).

I will file a follow-up bug to get the TSan build added per-push, then we can work on the races showing up in tests.

Blocks: 1132519
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2fbdd8a20529
Use builtin suppression lists for LSan and TSan. r=jseward
https://hg.mozilla.org/integration/autoland/rev/71ff8b5daaf6
Remove LSan/UBSan suppressions option from automation. r=ahal
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19734 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/19734
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/V5_3VnZ1S8KyRkcUQMOh6A)
Flags: needinfo?(choller)

At first glance, it looks like the wpt tests use an out-of-date version of testing/mozbase/mozrunner/mozrunner/utils.py (which is modified in this patch).

:ahal, do you know who would be the right person to talk to about this?

Flags: needinfo?(choller) → needinfo?(ahal)

Redirecting to James. I'm not sure how updates to mozbase dependencies are supposed to be handled.

Flags: needinfo?(ahal) → needinfo?(james)
Regressions: 1589207
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Well if you're making a change to mozbase you need to bump the version number, arrange for a release to happen (file a bug in the mozbase compoent with a patch for the version number bump and a needinfo to a module owner requesting a release), and push an additional patch to this bug to bump the requirements file at https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_firefox.txt#9. That will update the PR and hopefully allow things to pass.

I can help with the release; if you also need help with the other steps just let me know.

Flags: needinfo?(james) → needinfo?(choller)

Re-opening based on comment 11 since follow-up work is required.

Status: RESOLVED → REOPENED
Flags: needinfo?(choller)
Resolution: FIXED → ---
Depends on: 1589413

It seems like this patch might have caused me to get this error now:

KeyError: u'lsan_dir'

  File "/home/visbrero/rev_control/hg/mozilla-unified/testing/web-platform/mach_commands.py", line 312, in run_wpt
    return self.run_web_platform_tests(**params)
  File "/home/visbrero/rev_control/hg/mozilla-unified/testing/web-platform/mach_commands.py", line 304, in run_web_platform_tests
    return wpt_runner.run(logger, **params)
  File "/home/visbrero/rev_control/hg/mozilla-unified/testing/web-platform/mach_commands_base.py", line 33, in run
    kwargs = self.setup.kwargs_firefox(kwargs)
  File "/home/visbrero/rev_control/hg/mozilla-unified/testing/web-platform/mach_commands.py", line 92, in kwargs_firefox
    kwargs = self.kwargs_common(kwargs)
  File "/home/visbrero/rev_control/hg/mozilla-unified/testing/web-platform/mach_commands.py", line 79, in kwargs_common
    if kwargs["lsan_dir"] is None:

If I understand correctly, the still present code at https://hg.mozilla.org/mozilla-central/file/tip/testing/web-platform/mach_commands.py#l79

        if kwargs["lsan_dir"] is None:
            kwargs["lsan_dir"] = os.path.join(self.topsrcdir, "build", "sanitizers")

Is depending on the arguments dictionary to have had a "lsan_dir" option added by the now removed hunk of code at:
https://hg.mozilla.org/mozilla-central/rev/71ff8b5daaf686cbd578d260e4a1683c1ccab7ae#l9.12

-    gecko_group.add_argument("--lsan-dir", dest="lsan_dir", action="store",
-                             help="Path to directory containing LSAN suppressions file")

Comment 13 can be reproduced by running ./mach wpt, and deleting the following two lines in testing/web-platform/mach_commands.py works for me.

        if kwargs["lsan_dir"] is None:
            kwargs["lsan_dir"] = os.path.join(self.topsrcdir, "build", "sanitizers")
Regressions: 1589612
Regressions: 1556638
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37a84a2d91ff
Bump mozrunner version for web-platform tests. r=jgraham
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: mozilla71 → mozilla72
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.