Closed Bug 1509848 Opened 2 years ago Closed 2 years ago

Some basic Cranelift maintenance

Categories

(Core :: Javascript: WebAssembly, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

A few cleanups I've done and never submitted, so I might as well open a bug for this.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #9027521 - Flags: review?(nfroyd)
Warnings are actually good because they show dead code. Also rejiggered a bit imports.
Attachment #9027526 - Flags: review?(sunfish)
Very small patch to add an assertion that we don't use Cranelift for asm.js (was true already, just wanted to assert it).
Attachment #9027527 - Flags: review?(lhansen)
Attached patch 4.clippy.patchSplinter Review
Clippy provides even more warnings, in particular make functions unsafe instead of having unsafe blocks.
Attachment #9027528 - Flags: review?(sunfish)
Attachment #9027527 - Flags: review?(lhansen) → review+
Comment on attachment 9027521 [details] [diff] [review]
1.allow-compile-without-spidermonkey.patch

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

r=me, I guess, but I'm not completely confident that this actually works everywhere besides Linux?

::: js/src/wasm/cranelift/build.rs
@@ +50,5 @@
>              "-DRUST_BINDGEN",
>          ]).clang_arg("-I../..");
>  
> +    match env::var_os("MOZ_TOPOBJDIR") {
> +        Some(path) => {

I think it'd be worth a short comment here explaining why it's OK if this environment variable doesn't exist.

Actually...why is it OK?  Won't bindgen fall over if the flags we're trying to add aren't present?
Attachment #9027521 - Flags: review?(nfroyd) → review+
> Actually...why is it OK?  Won't bindgen fall over if the flags we're trying to add aren't present?

You're right, it'll work only on platforms which don't require additional bindgen flags. Well, it's certainly enough for me, might not be for others. I'd like to keep it though, since it's quite useful; should i also emit a warning if the MOZ_TOPOBJDIR variable isn't set, telling that this is not the expected way to compile Baldrdash?
Flags: needinfo?(nfroyd)
Comment on attachment 9027528 [details] [diff] [review]
4.clippy.patch

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

Clippy all the things!
Attachment #9027528 - Flags: review?(sunfish) → review+
Comment on attachment 9027526 [details] [diff] [review]
2.fix-warnings.patch

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

Yeah, I suspect allowing dead code is an artifact of an earlier era for this code.
Attachment #9027526 - Flags: review?(sunfish) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> > Actually...why is it OK?  Won't bindgen fall over if the flags we're trying to add aren't present?
> 
> You're right, it'll work only on platforms which don't require additional
> bindgen flags. Well, it's certainly enough for me, might not be for others.
> I'd like to keep it though, since it's quite useful; should i also emit a
> warning if the MOZ_TOPOBJDIR variable isn't set, telling that this is not
> the expected way to compile Baldrdash?

That seems reasonable, though I don't know if that warning would be surfaced properly through RLS or similar.  Is it possible to try and fish around for the necessary file if we don't know where MOZ_TOPOBJDIR is, or is that doomed to failure since `cargo` or similar is trying to run in the srcdir in the RLS case?  (I guess we could try to figure out topsrcdir, and then look into the mach-default obj-* directory name as a feeble attempt at supporting this case?)
See comment 9, thx to mid-airing with sunfish.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #9)
> That seems reasonable, though I don't know if that warning would be surfaced
> properly through RLS or similar.  Is it possible to try and fish around for
> the necessary file if we don't know where MOZ_TOPOBJDIR is, or is that
> doomed to failure since `cargo` or similar is trying to run in the srcdir in
> the RLS case?  (I guess we could try to figure out topsrcdir, and then look
> into the mach-default obj-* directory name as a feeble attempt at supporting
> this case?)

The problem is that the MOZ_TOPOBJDIR really could be anywhere; we only get the path to the Cargo manifest of the baldrdash library. So we could look for the default obj-* directory, but that wouldn't even work for me (I don't build Firefox there, for one). I think we can just issue the warning in this case; the worse that can happen is that building from this directory won't work (and RLS will just not say anything in this case).
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcec7e5baa7
Allow Baldrdash to be compiled independently from Spidermonkey; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf218f8c16a
Reenable warnings in Baldrdash and fix them; r=sunfish
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc064fbbcdf0
Cleanups in Baldrdash code; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c94d53b486c
Run clippy on Baldrdash; r=sunfish
> (In reply to Nathan Froyd [:froydnj] from comment #9)
> 
> The problem is that the MOZ_TOPOBJDIR really could be anywhere; we only get
> the path to the Cargo manifest of the baldrdash library. So we could look
> for the default obj-* directory, but that wouldn't even work for me (I don't
> build Firefox there, for one). I think we can just issue the warning in this
> case; the worse that can happen is that building from this directory won't
> work (and RLS will just not say anything in this case).

I just hit a compile error on macOS due to this, and there was no warning emitted. The error I got was:

error: failed to run custom build command for `baldrdash v0.1.0 (/Users/brad/repos/stylo/js/src/wasm/cranelift)`

It was only through some searching on Bugzilla that I found this bug. Of course, a regression check would have found the issue. But the error message makes no mention of setting MOZ_TOPOBJDIR, which appears to be the solution. Would you please consider a follow-on bug that improves the error reporting? I'd file it myself but I'm not confident I can characterize the issue correctly.
Flags: needinfo?(bbouvier)
Can you confirm there were not any other lines in the error message you got, please? Also, what build commands did you use? Using `mach build` or the regular way to build Spidermonkey (obj dir then configure then make), I can't see how one could run into an issue caused by this patch.
Flags: needinfo?(bbouvier) → needinfo?(bwerth)
Sure. I actually am stilling getting the error after setting MOZ_TOPOBJDIR, so I don't know that I'm bothering the right person about the right issue. But, here's the error log some ways along in a "mach build" run:

 5:28.31    Compiling baldrdash v0.1.0 (/Users/brad/repos/stylo/js/src/wasm/cranelift)
 5:28.98 error: failed to run custom build command for `baldrdash v0.1.0 (/Users/brad/repos/stylo/js/src/wasm/cranelift)`
 5:28.98 process didn't exit successfully: `/Users/brad/repos/obj-stylo/debug/build/baldrdash-acdcac4b8adef60e/build-script-build` (exit code: 101)
 5:28.98 --- stdout
 5:28.98 cargo:rerun-if-changed=baldrapi.h
 5:28.98 cargo:rerun-if-changed=/Users/brad/repos/obj-stylo/js/src/rust/extra-bindgen-flags
 5:28.98 --- stderr
 5:28.98 /opt/local/libexec/llvm-4.0/lib/clang/4.0.1/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found
 5:28.98 /opt/local/libexec/llvm-4.0/lib/clang/4.0.1/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found, err: true
 5:28.98 thread 'main' panicked at 'Unable to generate baldrapi.h bindings: ()', src/libcore/result.rs:1009:5
 5:28.98 stack backtrace:
 5:28.98    0:        0x10d0072f3 - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::h4a2f945c17ba2811
 5:28.98    1:        0x10cffebbc - std::sys_common::backtrace::_print::ha9b171449ed45096
 5:28.98    2:        0x10d003d01 - std::panicking::default_hook::{{closure}}::hfb91a8b1e6ba36f6
 5:28.98    3:        0x10d0039fd - std::panicking::default_hook::h345fdd8c0726903f
 5:28.98    4:        0x10d00448e - std::panicking::rust_panic_with_hook::h60b2c7e9825136db
 5:28.98    5:        0x10d003fac - std::panicking::continue_panic_fmt::h527fc4c4b865a925
 5:28.98    6:        0x10d003e98 - rust_begin_unwind
 5:28.98    7:        0x10d02b031 - core::panicking::panic_fmt::h1c4bf1dd8124a23a
 5:28.99    8:        0x10b874797 - core::result::unwrap_failed::hb56f589f9f37bc95
 5:28.99                                at /rustc/b68fc18c45350e1cdcd83cecf0f12e294e55af56/src/libcore/macros.rs:26
 5:28.99    9:        0x10b872b54 - <core::result::Result<T, E>>::expect::hed344a13a1495364
 5:28.99                                at /rustc/b68fc18c45350e1cdcd83cecf0f12e294e55af56/src/libcore/result.rs:835
 5:28.99   10:        0x10b874523 - build_script_build::main::hf5b13913c717a43e
 5:28.99                                at js/src/wasm/cranelift/build.rs:79
 5:28.99   11:        0x10b870a55 - std::rt::lang_start::{{closure}}::hea91c7f4bccfa921
 5:28.99                                at /rustc/b68fc18c45350e1cdcd83cecf0f12e294e55af56/src/libstd/rt.rs:74
 5:28.99   12:        0x10d003e17 - std::panicking::try::do_call::hf2425ad51017352e
 5:28.99   13:        0x10d01fe8e - __rust_maybe_catch_panic
 5:28.99   14:        0x10d00486a - std::rt::lang_start_internal::hea882f3b2530ace7
 5:28.99   15:        0x10b870a41 - std::rt::lang_start::h5c03c7eaeb6cb934
 5:28.99                                at /rustc/b68fc18c45350e1cdcd83cecf0f12e294e55af56/src/libstd/rt.rs:74
 5:28.99 make[4]: *** [force-cargo-library-build] Error 101
 5:28.99 make[3]: *** [toolkit/library/rust/target] Error 2
 5:28.99 make[3]: *** Waiting for unfinished jobs....
Flags: needinfo?(bwerth)
Either you're hitting bug 1487552 if you're using Mojave, or the build step isn't building the extra-bindgen-flags. Can you check if the former helps? Otherwise, can you please open a new bug in the Build system component and cc me? Thanks!
Flags: needinfo?(bwerth)
(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> Either you're hitting bug 1487552 if you're using Mojave, or the build step
> isn't building the extra-bindgen-flags. Can you check if the former helps?
> Otherwise, can you please open a new bug in the Build system component and
> cc me? Thanks!

Yes, I am hitting the issue in bug 1487552, but the fix there (manually install the command line tools) does not solve the problem. Xidorn has a bug filed at bug 1509696 that shows his local fix, which I will adopt for now.
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.