1335411 - Mac cross-compile does not build with Address Sanitizer

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jesse Schwartzentruber (:truber)]

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.

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: bug1335411.patch

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Jesse Schwartzentruber (:truber)]

Attachment: Update with review feedback.

(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.

Comment 5

[Nathan Froyd [:froydnj]]

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!

Comment 6

[Jesse Schwartzentruber (:truber)]

Thanks Nathan!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e88c7e7676c