1335203 - Synchronize rust debug_assertions flags with the DEBUG define.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

In bug 1334579 I landed a patch that that added a DEBUG-only function to integrate with leak-logging, and guarded the call from that function in rust with #[cfg(debug_assertions)].

Apparently, the DEBUG ifdef and the rust cfg aren't in sync, so this broke some release builds for some people with an linker error like:

Undefined symbols for architecture x86_64:
 0:40.83   "_Gecko_IncrementStringAdoptCount", referenced from:
 0:40.83       _$LT$nsstring..nsCString$LT$$u27$static$GT$$u20$as$u20$core..convert..From$LT$Box$LT$$u5b$u8$u5d$$GT$$GT$$GT$::from::haf3d6c1780591d80 in libgkrust.a(gkrust.0.o)
 0:40.83       _$LT$nsstring..nsString$LT$$u27$static$GT$$u20$as$u20$core..convert..From$LT$Box$LT$$u5b$u16$u5d$$GT$$GT$$GT$::from::h0d4e119095c9886d in libgkrust.a(gkrust.0.o)

I'm landing a quick workaround by removing the ifdef, but we do this sort of stuff for stylo too, and seems like these two options should be defined together all the time?

Comment 1

[Bobby Holley (:bholley)]

Hm, this seems odd. Looking at [1], I don't see any reason why these wouldn't be in sync. Ted/Nathan, thoughts?

[1] http://searchfox.org/mozilla-central/source/toolkit/library/rust/Cargo.toml

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> Hm, this seems odd. Looking at [1], I don't see any reason why these
> wouldn't be in sync. Ted/Nathan, thoughts?
> 
> [1]
> http://searchfox.org/mozilla-central/source/toolkit/library/rust/Cargo.toml

It's possible that people were building with a CARGOFLAGS that overrode what we expected to see, and that caused issues?

Emilio, do you have mozconfigs for the people that were seeing problems?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Redirecting ni? to :haik which is the one that hit this.

Comment 4

[Haik Aftandilian [:haik]]

I did one thing that might make my configuration atypical: a few weeks ago I installed nightly rust to test something unrelated and then reverted to stable. I don't have anything setting CARGOFLAGS in my environment. And this was on Mac. Anything else I can check?

Here's my mozconfig.

  mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-nd.noindex
  mk_add_options AUTOCLOBBER=1
  mk_add_options AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213
  export MOZ_TELEMETRY_REPORTING=1
  ac_add_options --disable-optimize
  ac_add_options --enable-debug-symbols
  ac_add_options --disable-install-strip
  ac_add_options --disable-crashreporter
  ac_add_options --with-ccache
  ac_add_options --enable-dtrace
  ac_add_options --enable-tests
  ac_add_options --enable-eme=widevine

Rust install:

  $ rustup show
  Default host: x86_64-apple-darwin

  installed toolchains
  --------------------

  stable-x86_64-apple-darwin (default)
  nightly-x86_64-apple-darwin

  active toolchain
  ----------------

  stable-x86_64-apple-darwin (default)
  rustc 1.14.0 (e8a012324 2016-12-16)

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Haik Aftandilian [:haik] from comment #4)
> I did one thing that might make my configuration atypical: a few weeks ago I
> installed nightly rust to test something unrelated and then reverted to
> stable. I don't have anything setting CARGOFLAGS in my environment. And this
> was on Mac. Anything else I can check?

I don't think the switching back and forth should affect anything, but having --disable-optimize without --{enable,disable}-debug in your mozconfig is a bit peculiar.  Can you run:

1. grep DEBUG $OBJDIR/config.status; and
2. make -C $OBJDIR/toolkit/library/rust echo-variable-CARGO_BUILD

in an objdir configured with the mozconfig that you pasted in comment 4, and report those results?

Comment 6

[Haik Aftandilian [:haik]]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> (In reply to Haik Aftandilian [:haik] from comment #4)
> > I did one thing that might make my configuration atypical: a few weeks ago I
> > installed nightly rust to test something unrelated and then reverted to
> > stable. I don't have anything setting CARGOFLAGS in my environment. And this
> > was on Mac. Anything else I can check?
> 
> I don't think the switching back and forth should affect anything, but
> having --disable-optimize without --{enable,disable}-debug in your mozconfig
> is a bit peculiar.

OK.

> Can you run:
> 
> 1. grep DEBUG $OBJDIR/config.status; and
> 2. make -C $OBJDIR/toolkit/library/rust echo-variable-CARGO_BUILD
> 
> in an objdir configured with the mozconfig that you pasted in comment 4, and
> report those results?

$ grep DEBUG obj-nd.noindex/config.status
    'DEBUG_JS_MODULES': '',
    'MOZ_DEBUG': '',
    'MOZ_DEBUG_DEFINES': [
        'NDEBUG',
    'MOZ_DEBUG_FLAGS': '-g',
    'MOZ_DEBUG_LDFLAGS': ' -framework ExceptionHandling',
    'MOZ_DEBUG_SYMBOLS': '1',
    'MOZ_NO_DEBUG_RTL': '1',

$ make -C obj-nd.noindex/toolkit/library/rust echo-variable-CARGO_BUILD
env RUSTFLAGS='-C opt-level=0' CARGO_TARGET_DIR=. RUSTC=/Users/haftandilian/.cargo/bin/rustc MOZ_DIST=/Users/haftandilian/r/mozilla-central/obj-nd.noindex/dist LIBCLANG_PATH= CLANG_PATH= /Users/haftandilian/.cargo/bin/cargo build --release --frozen --manifest-path /Users/haftandilian/r/mozilla-central/toolkit/library/rust/Cargo.toml

Comment 7

[Nathan Froyd [:froydnj]]

OK, that's all as expected.  But that's still weird, because:

1. Cargo is receiving --release, so it should be using profile.release;
2. profile.release specifies debug-assertions = false;
3. we still compile code that's cfg(debug_assertions)

Comment 0 says that we're referencing what looks like an ordinary C symbol, but only the debug_assertions _Gecko_IncrementStringAdoptCount is declared |extern "C"|; the debug_assertions one isn't declared |extern "C"|, so I would assume it would be subject to rust name mangling/calling convention/etc.  Unless the rustc mangling here is identical to the C mangling, i.e. none...

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> OK, that's all as expected.  But that's still weird, because:
> 
> 1. Cargo is receiving --release, so it should be using profile.release;
> 2. profile.release specifies debug-assertions = false;
> 3. we still compile code that's cfg(debug_assertions)
> 
> Comment 0 says that we're referencing what looks like an ordinary C symbol,
> but only the debug_assertions _Gecko_IncrementStringAdoptCount is declared
> |extern "C"|; the debug_assertions one isn't declared |extern "C"|, so I
> would assume it would be subject to rust name mangling/calling
> convention/etc.  Unless the rustc mangling here is identical to the C
> mangling, i.e. none...

The non `extern "C"` version is defined in rust-land, it is simply written like that such that the call to the method will be allowed even when debug_assertions isn't set. It shouldn't link against the C++ version unless debug assertions are enabled.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #8)
> (In reply to Nathan Froyd [:froydnj] from comment #7)
> > OK, that's all as expected.  But that's still weird, because:
> > 
> > 1. Cargo is receiving --release, so it should be using profile.release;
> > 2. profile.release specifies debug-assertions = false;
> > 3. we still compile code that's cfg(debug_assertions)
> > 
> > Comment 0 says that we're referencing what looks like an ordinary C symbol,
> > but only the debug_assertions _Gecko_IncrementStringAdoptCount is declared
> > |extern "C"|; the debug_assertions one isn't declared |extern "C"|, so I
> > would assume it would be subject to rust name mangling/calling
> > convention/etc.  Unless the rustc mangling here is identical to the C
> > mangling, i.e. none...
> 
> The non `extern "C"` version is defined in rust-land, it is simply written
> like that such that the call to the method will be allowed even when
> debug_assertions isn't set. It shouldn't link against the C++ version unless
> debug assertions are enabled.

Right, I understand that part.  The linker is complaining about (comment 0):

Undefined symbols for architecture x86_64:
 0:40.83   "_Gecko_IncrementStringAdoptCount", referenced from:

which is the symbol name for an |extern "C"| symbol from C++ or Rust.  The question is whether, if you have a non-|extern "C"| Rust function, |Gecko_IncrementStringAdoptCount|, is the symbol name that the linker wants to link against |_Gecko_IncrementStringAdoptCount|, or is it something else?  Because if the answer is "something else", then the linker is trying to link against the |extern "C"| symbol in the non-debug_assertions case.  Which is exactly the case that the code as written is trying to avoid.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> Right, I understand that part.  The linker is complaining about (comment 0):
> 
> Undefined symbols for architecture x86_64:
>  0:40.83   "_Gecko_IncrementStringAdoptCount", referenced from:
> 
> which is the symbol name for an |extern "C"| symbol from C++ or Rust.  The
> question is whether, if you have a non-|extern "C"| Rust function,
> |Gecko_IncrementStringAdoptCount|, is the symbol name that the linker wants
> to link against |_Gecko_IncrementStringAdoptCount|, or is it something else?
> Because if the answer is "something else", then the linker is trying to link
> against the |extern "C"| symbol in the non-debug_assertions case.  Which is
> exactly the case that the code as written is trying to avoid.

Compiling just the definition of Gecko_IncrementStringAdoptCount() in the rust playpen suggests that the symbol name which would be used is: _ZN8rust_out31Gecko_IncrementStringAdoptCount17hf76a810266e7a0ffE or similar (the rust_out would probably say nsstring instead).

In other words, we're trying to link against the extern "C" symbol.

Comment 11

[Nathan Froyd [:froydnj]]

Thanks for testing that.

OK, so that seems bad.  Haik, are you positive that you don't have anything squirreled away in your environment that might be affecting this?  Maybe post the output of `env` if you can?

Comment 12

[Haik Aftandilian [:haik]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> Thanks for testing that.
> 
> OK, so that seems bad.  Haik, are you positive that you don't have anything
> squirreled away in your environment that might be affecting this?  Maybe
> post the output of `env` if you can?

I can't find anything suspect. The only non-standard thing I have is /usr/local/bin early in the PATH.

$ env
TERM_PROGRAM=Apple_Terminal
TERM=xterm-256color
SHELL=/bin/bash
TMPDIR=/var/folders/46/XYZ/T/
Apple_PubSub_Socket_Render=/private/tmp/com.apple.launchd.XYZ/Render
TERM_PROGRAM_VERSION=388
TERM_SESSION_ID=XYZ
USER=haftandilian
SSH_AUTH_SOCK=/private/tmp/com.apple.launchd.XYZ/Listeners
__CF_USER_TEXT_ENCODING=0x1F5:0x0:0x0
PATH=/Users/haftandilian/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/haftandilian/bin
PWD=/Users/haftandilian/r/mozilla-central
LANG=en_US.UTF-8
XPC_FLAGS=0x0
PS1=\w \$ 
XPC_SERVICE_NAME=0
HOME=/Users/haftandilian
SHLVL=1
LESS=-R
LOGNAME=haftandilian
_=/usr/bin/env

Comment 13

[Haik Aftandilian [:haik]]

Without "ac_add_options --disable-optimize", my build works.

This works

  mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-nd.noindex
  mk_add_options AUTOCLOBBER=1
  mk_add_options AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213
  export MOZ_TELEMETRY_REPORTING=1

while this does not

  mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-nd.noindex
  mk_add_options AUTOCLOBBER=1
  mk_add_options AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213
  export MOZ_TELEMETRY_REPORTING=1
  ac_add_options --disable-optimize

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I can reproduce this link failure locally. 

When --disable-optimize is run, the following command is executed:

/home/mlayzell/.cargo/bin/rustc --crate-name nsstring /home/mlayzell/Code/moz/central/xpcom/rust/nsstring/src/lib.rs --color always --crate-type lib --emit=dep-info,link -C opt-level=2 -C panic=abort -C codegen-units=1 -C debuginfo=2 -C metadata=70471bf591d0253f -C extra-filename=-70471bf591d0253f --out-dir /home/mlayzell/Code/moz/central/obj-noopt-x86_64-pc-linux-gnu/toolkit/library/gtest/rust/./x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/home/mlayzell/Code/moz/central/obj-noopt-x86_64-pc-linux-gnu/toolkit/library/gtest/rust/./x86_64-unknown-linux-gnu/release/deps -L dependency=/home/mlayzell/Code/moz/central/obj-noopt-x86_64-pc-linux-gnu/toolkit/library/gtest/rust/./release/deps -C opt-level=0

This appears to set cfg(debug_assertions), despite not having -C debug-assertions passed on the command line, as after commenting out the #[cfg(not(debug_assertions))] declaration, it still compiles.

The interesting thing it that if I remove the final "-C opt-level=0" debug_assertions is not set, and the build fails, complaining that it cannot find a function named Gecko_IncrementStringAdoptCount.

Effectively, it seems like debug_assertions is implied by -C opt-level=0 for rustc. This seems like a bug to me.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

From looking at the rust reference, it seems like this is intentional:
    debug_assertions - Enabled by default when compiling without optimizations. This can be used to enable extra debugging code in development but not in production. For example, it controls the behavior of the standard library's debug_assert! macro.

- I'll add some code to make sure we pass -C debug-assertions=no when passing -C opt-level=0 through RUSTFLAGS.

https://github.com/rust-lang/rust/blob/17cae033787aca26b00a8c820c1599926a1fd94d/src/doc/reference.md#conditional-compilation

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #15)
> From looking at the rust reference, it seems like this is intentional:
>     debug_assertions - Enabled by default when compiling without
> optimizations. This can be used to enable extra debugging code in
> development but not in production. For example, it controls the behavior of
> the standard library's debug_assert! macro.

Doh.  Thanks for looking into this!

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Synchronize rust debug-assertions with C++ DEBUG

MozReview-Commit-ID: JPD9eNFg89S

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8832555 [details] [diff] [review]
Synchronize rust debug-assertions with C++ DEBUG

Review of attachment 8832555 [details] [diff] [review]:
-----------------------------------------------------------------

r=me; I would prefer the change below, but I'm not going to be picky about it.

::: config/rules.mk
@@ +945,4 @@
>  ifndef MOZ_OPTIMIZE
> +ifndef MOZ_DEBUG
> +rustflags_override = RUSTFLAGS='-C opt-level=0 -C debug-assertions=no'
> +else

WDYT about:

ifndef MOZ_OPTIMIZE
rustflags = -C opt-level=0
# Can move the explanatory comment here.
ifndef MOZ_DEBUG
rustflags += -C debug-assertions=no
endif
rustflags_override = RUSTFLAGS='$(rustflags)'
endif # MOZ_OPTIMIZE

so we're not repeating ourselves?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc329efa94a9
Synchronize rust debug-assertions with C++ DEBUG, r=froydnj

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc329efa94a9