Opened 14 months ago

Closed 9 months ago

Last modified 8 months ago

#27597 closed defect (fixed)

ASan build option in tor-browser-build is broken

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-8.0-issues, tbb-regression, tbb-rbm, TorBrowserTeam201901R
Cc: Actual Points:
Parent ID: Points:
Reviewer: boklm Sponsor:

Description

Trying to do a ASan build for #27531 results in build breakage due to --enable-tests and --enable-debug. In the first case we get

55:51.29 /var/tmp/build/firefox-124fa904c4b2/mfbt/tests/TestWrappingOperations.cpp:27:1: error: non-constant condition for static assertion
55:51.29  static_assert(WrapToSigned(uint8_t(128)) == -128,
55:51.29  ^~~~~~~~~~~~~
55:51.29 /var/tmp/build/firefox-124fa904c4b2/mfbt/tests/TestWrappingOperations.cpp:27:27:   in constexpr expansion of 'mozilla::WrapToSigned<unsigned char>(128)'
55:51.29 /var/tmp/build/firefox-124fa904c4b2/obj-x86_64-pc-linux-gnu/dist/include/mozilla/WrappingOperations.h:89:59:   in constexpr expansion of 'mozilla::detail::WrapToSignedHelper<UnsignedType>::compute<unsigned char>(((int)aValue))'

and similar errors. Disabling tests results in packaging errors

 0:01.00 /usr/bin/make -j4 -s stage-package
 0:01.71 Error: /var/tmp/build/firefox-124fa904c4b2/browser/installer/package-manifest.in:201: Missing file(s): bin/components/dom_bindings_test.xpt
 0:01.75 Error: /var/tmp/build/firefox-124fa904c4b2/browser/installer/package-manifest.in:246: Missing file(s): bin/components/layout_debug.xpt

Child Tickets

Change History (7)

comment:1 Changed 10 months ago by gk

Keywords: TorBrowserTeam201812R added
Status: newneeds_review

Okay, the packaging part at least was so annoying that I wrote a fix for that to get more information about WebGL related crashes. bug_27597 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_27597) in my `tor-browser has two patches in case we want to get this upstreamed to esr60. Here is the issue:

dom/bindings/moz.build contains

TEST_DIRS += ['test']

python/mozbuild/mozbuild/frontend/context.py has

    'TEST_DIRS': (lambda context: context['DIRS'] if context.config.substs.get('ENABLE_TESTS')
                                  else TestDirsPlaceHolder, list,
        """Like DIRS but only for directories that contain test-only code.

        If tests are not enabled, this variable will be ignored.

so, test is ignored with --disable-tests, thus

if CONFIG['MOZ_DEBUG']:
    XPIDL_SOURCES += [
        'mozITestInterfaceJS.idl',
    ]

    XPIDL_MODULE = 'dom_bindings_test'

in dom/bindings/test/moz.build is not executed which breaks

#if defined(MOZ_DEBUG)
@RESPATH@/components/dom_bindings_test.xpt
#endif

if --enable-debug is set.

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1013882 for a similar issue.

Introduced by https://bugzilla.mozilla.org/show_bug.cgi?id=1424362
(https://hg.mozilla.org/mozilla-central/rev/b1133d2f2c66).

layout/moz.build contains

if CONFIG['MOZ_DEBUG'] and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
    TEST_DIRS += ['tools/layout-debug']

but, as explained above, TEST_DIRS is ignored if --disable-tests is set. Thus
this breaks

#ifdef MOZ_DEBUG
@BINPATH@/components/layout_debug.xpt
#endif

if --enable-debug is set.

Introduced by https://bugzilla.mozilla.org/show_bug.cgi?id=1417664
(https://hg.mozilla.org/releases/mozilla-esr60/rev/07d02e9d114c).

Setting needs_review for the --enable-debug related part.

comment:2 Changed 10 months ago by gk

Okay, I pushed another commit (commit 570ae84b5182a7d6986e467540318e5282fe0f2e) that fixes the --enable-tests related problem. It turns out that fwrapv does not like the code added in https://bugzilla.mozilla.org/show_bug.cgi?id=1432646 and breaks. I have not looked closer whether that's a compiler bug or something else. For now I just filed an upstream bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1514794) and removed the -fwrapv flag. Which is okayish for our debug build I think.

comment:3 Changed 10 months ago by gk

Keywords: TorBrowserTeam201901R added; TorBrowserTeam201812R removed

Moving review tickets to 2019.

comment:4 Changed 9 months ago by gk

Reviewer: boklm

comment:5 Changed 9 months ago by boklm

Status: needs_reviewmerge_ready

The three patches look good to me.

comment:6 Changed 9 months ago by gk

Resolution: fixed
Status: merge_readyclosed

Thanks, I applied them to tor-browser-60.4.0esr-8.5-1 as commit
2d5a66c40885598133593c641fd8401a0b2f2fac,
3723fd7b92f17ed857cea9bacb26b77fa4d34efc, and
b5793205da5fde6035f1bb7c49752fd38a9ed6e9.

comment:7 in reply to:  2 Changed 8 months ago by gk

Replying to gk:

Okay, I pushed another commit (commit 570ae84b5182a7d6986e467540318e5282fe0f2e) that fixes the --enable-tests related problem. It turns out that fwrapv does not like the code added in https://bugzilla.mozilla.org/show_bug.cgi?id=1432646 and breaks. I have not looked closer whether that's a compiler bug or something else.

Seems indeed to be a compiler bug. GCC >= 8.1 has no issue with it.

Last edited 8 months ago by gk (previous) (diff)
Note: See TracTickets for help on using tickets.