1320179 - Provide rust bindings to nsresult

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Rust binding code should be able to refer to a nsresult rust crate which has a FFI safe nsresult type, and consts declared for each of the nsresult error types.

The crate may also wish to provide basic translations between common rust Result<T, E> and Gecko nsresult types in the form of helper macros or traits.

This would help with, for example, simplifying the rust_uri_capi crate.

Comment 1

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

Attachment: Part 1: Move nsresult value calculation into a python script

This patch makes the error codes in nsError.h be generated by a python script.
This gives us the opportunity to add rust code generation in parallel with the
C++ code generation, which will happen in part 2.

This patch also reworks the name calculation in ErrorNames.cpp to use generated
code by the python script.

MozReview-Commit-ID: 5wxbdZwxe7q

Comment 2

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

Attachment: Part 2: Add the nserror rust crate and generate NS_ERROR codes

MozReview-Commit-ID: FFU6WfEqev

Comment 3

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

If you want to see an example of this crate in use, check out bug 1352553 where I use it to simplify the error handling in rust_url_capi.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

So the problem with this is that this will break the 32-bit ABI. Returning nsresult from an FFI function will return a struct from rust's point of view, while C++ expects an enum value.

See https://github.com/servo/rust-bindgen/issues/439 for an example of this. https://github.com/rust-lang/rfcs/pull/1758 is the solution for this.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8853534 [details] [diff] [review]
Part 1: Move nsresult value calculation into a python script

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

r=me, but can you attach a copy of the generated ErrorList.h file so I can look over it?  I didn't review the transfer of errors between the .h and the .py closely, and I think it might be easier to review the actual .h file for something like that.  Thanks!

::: xpcom/base/ErrorList.py
@@ +28,5 @@
> +# NE_ERROR_GENERATExxxxxx macros.  Some examples below:
> +#
> +#    #define NS_ERROR_MYMODULE_MYERROR1 NS_ERROR_GENERATE(NS_ERROR_SEVERITY_ERROR,NS_ERROR_MODULE_MYMODULE,1)
> +#    #define NS_ERROR_MYMODULE_MYERROR2 NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_MYMODULE,2)
> +#    #define NS_ERROR_MYMODULE_MYERROR3 NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_MYMODULE,3)

Does this comment actually apply to this brave new world?

@@ +1214,5 @@
> +
> +    seen = set()
> +    for error, val in errors.iteritems():
> +        if val not in seen:
> +            output.write('  if (((uint32_t) rv) == {}) {{ return "{}"; }}\n'.format(val, error))

This generates some terrible code, but I guess we can clean that up in a followup bug.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8853535 [details] [diff] [review]
Part 2: Add the nserror rust crate and generate NS_ERROR codes

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

Thanks for tackling this.

::: config/rules.mk
@@ +953,5 @@
>  	LIBCLANG_PATH=$(MOZ_LIBCLANG_PATH) \
>  	CLANG_PATH=$(MOZ_CLANG_PATH) \
>  	PKG_CONFIG_ALLOW_CROSS=1 \
>  	RUST_BACKTRACE=1 \
> +	MOZ_OBJDIR=$(topobjdir) \

We should call this MOZ_TOPOBJDIR, since MOZ_OBJDIR might refer to the current objdir being traversed in some future world.

::: xpcom/base/ErrorList.py
@@ +1249,5 @@
> +
> +    seen = set()
> +    for error, val in errors.iteritems():
> +        if val not in seen:
> +            output.write('if self.code == {} {{ return "{}"; }}\n'.format(val, error))

It seems like it'd be better if this was shared code somehow between C++ and Rust.  I'm not particularly concerned about which side is the canonical one, but having two auto-generated functions that do basically the same thing doesn't seem like a win.

::: xpcom/rust/nserror/src/lib.rs
@@ +5,5 @@
> +#[repr(C)]
> +#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> +pub struct nsresult {
> +    pub code: u32,
> +}

As Emilio notes, this is not going to work in general, even if it might work on the majority of our platforms.

Comment 7

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

Attachment: Part 1: Move nsresult value calculation into a python script

Updated

MozReview-Commit-ID: 5wxbdZwxe7q

Comment 8

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

Attachment: Part 2: Add the nserror rust crate and generate NS_ERROR codes

MozReview-Commit-ID: FFU6WfEqev

Comment 9

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

Attachment: ErrorList.h

This is the generated value of ErrorList.h, for your convenient perusal.

Comment 10

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

Attachment: ErrorNamesInternal.h

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8854506 [details] [diff] [review]
Part 2: Add the nserror rust crate and generate NS_ERROR codes

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

r=me with the change below.

::: config/rules.mk
@@ +953,5 @@
>  	LIBCLANG_PATH=$(MOZ_LIBCLANG_PATH) \
>  	CLANG_PATH=$(MOZ_CLANG_PATH) \
>  	PKG_CONFIG_ALLOW_CROSS=1 \
>  	RUST_BACKTRACE=1 \
> +	MOZ_OBJDIR=$(topobjdir) \

MOZ_TOPOBJDIR, as requested previously, please.

Comment 12

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

Attachment: Part 3: Define LOGLEVEL_0 unconditionally in protobuf on windows

This brings the workaround for the GOOGLE_LOG(ERROR) problem when ERROR is
defined as `0` by wingdi.h in line with our workaround in the chromium sandbox
code. (http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/security/sandbox/chromium/base/logging.h#334-346)

The other patches on this bug triggered a build problem where in some situations
ERROR would be undefined when the LogLevel enum is being defined, while ERROR
was defined at a callsite. By unconditionally defining ERROR on windows and
including the LOGLEVEL_0 enum variant, we can avoid this issue and fix the
associated build bustage.

I'm not confident whether or not this is an acceptable workaround, given that it
modifies vendored code. The code which I'm changing already feels like a mozilla
patch on top of the protobuf library, so I think it should be fine...

MozReview-Commit-ID: 3XFHf1FqHBr

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8855045 [details] [diff] [review]
Part 3: Define LOGLEVEL_0 unconditionally in protobuf on windows

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

Make sure to roll these changes into our downstream roll up patch so we can more easily rebase on top of future protobuf lib updates.

http://searchfox.org/mozilla-central/source/toolkit/components/protobuf/m-c-changes.patch

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea4f1b3cc7b1
Part 1: Move nsresult value calculation into a python script, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/470d8143b350
Part 2: Add the nserror rust crate and generate NS_ERROR codes, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2f77570128
Part 3: Define LOGLEVEL_0 unconditionally in protobuf on windows, r=fitzgen

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Pulsebot from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2f77570128
> Part 3: Define LOGLEVEL_0 unconditionally in protobuf on windows, r=fitzgen

Gentle poke: this does not include the additional changes I requested in comment 13. Please land a follow up -- thanks!

Comment 16

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

Attachment: Part 4: MOZ_OBJDIR->MOZ_TOPOBJDIR and update m-c-changes.patch for protobuf changes

I'm so sorry I missed your feedback - I knew I was forgetting something when I looked at the try results and just pushed... :S

Thanks for pointing this out to me!

MozReview-Commit-ID: EPhkF350sGY

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8aa677f7452
Part 4: MOZ_OBJDIR->MOZ_TOPOBJDIR and update m-c-changes.patch for protobuf changes, r=fitzgen

Comment 18

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

I had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/80eddce40afc for causing merge conflicts for me trying to merge mozilla-central back over to inbound (due to Bug 1353944 being on m-c). Feel free to reland this.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3979bac65e1d
Part 1: Move nsresult value calculation into a python script, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a351f0aac3
Part 2: Add the nserror rust crate and generate NS_ERROR codes, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bcf8686408
Part 3: Define LOGLEVEL_0 unconditionally in protobuf on windows, r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/04c98671aaf8
Part 4: MOZ_OBJDIR->MOZ_TOPOBJDIR and update m-c-changes.patch for protobuf changes, r=fitzgen

Comment 20

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

Relanded

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3979bac65e1d
https://hg.mozilla.org/mozilla-central/rev/b1a351f0aac3
https://hg.mozilla.org/mozilla-central/rev/e1bcf8686408
https://hg.mozilla.org/mozilla-central/rev/04c98671aaf8

Comment 22

[Henri Sivonen (:hsivonen)]

This is great. Thank you.

Comment 23

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Do we need to touch CLOBBER to prevent other people accidentally add ErrorList.h back again? (e.g. as in bug 1352699)