Closed
Bug 1335411
Opened 6 years ago
Closed 6 years ago
Mac cross-compile does not build with Address Sanitizer
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: truber, Assigned: truber)
References
Details
Attachments
(1 file, 1 obsolete file)
6.84 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
debug-asan config does not work when cross-compiling per bug 921040. We should also have a nightly-asan config for optimized builds to mirror what is available for linux.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8832045 -
Flags: review?(ted)
Comment 2•6 years ago
|
||
Comment on attachment 8832045 [details] [diff] [review] bug1335411.patch I'm going to punt this review to Nathan, I have a bit of a review backlog that I'm trying to clear out and I don't want to leave you hanging. Nathan: if there's anything here you're not comfortable reviewing feel free to pass it back.
Attachment #8832045 -
Flags: review?(ted) → review?(nfroyd)
![]() |
||
Comment 3•6 years ago
|
||
Comment on attachment 8832045 [details] [diff] [review] bug1335411.patch Review of attachment 8832045 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking into this; r=me with some minor changes noted below. I'm ambivalent about rewriting the rewrite_asan_dylib.py loop, though. ::: browser/config/mozconfigs/macosx64/nightly-asan @@ +5,5 @@ > +ac_add_options --disable-debug > +ac_add_options --enable-optimize="-O2" > + > +# Package js shell. > +export MOZ_PACKAGE_JSSHELL=1 We don't enable telemetry reporting here, but we do in the debug version, which seems odd. Maybe we want to disable it in both, or just enable it in nightly? Oh, I see, telemetry reporting is enabled by macosx/mozconfig.common. So is the JS shell, for that matter...Maybe we should just remove those bits from debug-asan and nightly-asan? @@ +8,5 @@ > +# Package js shell. > +export MOZ_PACKAGE_JSSHELL=1 > + > +if test "${MOZ_UPDATE_CHANNEL}" = "nightly"; then > +ac_add_options --with-macbundlename-prefix=Firefox Nit: maybe you want to indent this, as it's inside an `if`? I guess this matches the debug one, though. ::: build/autoconf/toolchain.m4 @@ +82,4 @@ > AC_CHECK_PROGS(STRIP, "${TOOLCHAIN_PREFIX}strip", :) > AC_CHECK_PROGS(WINDRES, "${TOOLCHAIN_PREFIX}windres", :) > AC_CHECK_PROGS(OTOOL, "${TOOLCHAIN_PREFIX}otool", :) > +AC_CHECK_PROGS(INSTALL_NAME_TOOL, "${TOOLCHAIN_PREFIX}install_name_tool", :) TIL about install_name_tool. ::: build/unix/rewrite_asan_dylib.py @@ +22,5 @@ > + otoolOut = subprocess.check_output([substs['OTOOL'], '-l', filename]) > + cmd = None > + > + for line in otoolOut.splitlines(): > + Nit: extraneous blank line. @@ +26,5 @@ > + > + cmdMatch = re.match(r'^\s+cmd ([A-Z_]+)', line) > + if cmdMatch is not None: > + cmd = cmdMatch.group(1) > + continue This loop threw me when I first read it, but I'm not sure you can do any better, unless you did something like: lines = otoolOut.splitlines() for cmdline, pathline in itertools.izip(lines, lines[1:]): ... @@ +29,5 @@ > + cmd = cmdMatch.group(1) > + continue > + > + if cmd == 'LC_RPATH': > + Nit: extraneous blank line.
Attachment #8832045 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #3) Hi Nathan, thanks for the review! I've fixed most of your nits. My goal for the mozconfigs was to make diffs against the linux nightly-asan/debug-asan, and the macosx64 nightly/debug as small as possible. > We don't enable telemetry reporting here, but we do in the debug version, > which seems odd. Maybe we want to disable it in both, or just enable it in > nightly? > > Oh, I see, telemetry reporting is enabled by macosx/mozconfig.common. So is > the JS shell, for that matter...Maybe we should just remove those bits from > debug-asan and nightly-asan? Oh, you mean macosx64/common-opt? You're right, but I'm not sure we want everything there enabled for asan builds. The linux configs have the same common-opt structure, and linux64 asan configs don't do this. I agree the omission of telemetry in nightly-asan is probably a mistake in linux64/nightly-asan, so I enabled it for macosx64. > > +if test "${MOZ_UPDATE_CHANNEL}" = "nightly"; then > > +ac_add_options --with-macbundlename-prefix=Firefox > > Nit: maybe you want to indent this, as it's inside an `if`? I guess this > matches the debug one, though. I left this just to keep the diff easier with existing configs. > This loop threw me when I first read it, but I'm not sure you can do any > better, unless you did something like: > > lines = otoolOut.splitlines() > for cmdline, pathline in itertools.izip(lines, lines[1:]): > ... This won't work because "cmd" is the first of a block of lines describing that command. "path" won't immediately follow, but will be before the next "cmd". I've added a comment and renamed "cmd" to hopefully make it clearer.
Attachment #8832045 -
Attachment is obsolete: true
Attachment #8836142 -
Flags: review?(nfroyd)
![]() |
||
Comment 5•6 years ago
|
||
Comment on attachment 8836142 [details] [diff] [review] Update with review feedback. Review of attachment 8836142 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the changes and the explanation!
Attachment #8836142 -
Flags: review?(nfroyd) → review+
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1e88c7e7676c Fix --enable-address-sanitizer for Mac cross-compilation and adapt Linux ASan configs for Mac. r=froydnj
Keywords: checkin-needed
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1e88c7e7676c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•