Closed Bug 1320179 Opened 3 years ago Closed 3 years ago

Provide rust bindings to nsresult

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

Attachments

(6 files, 2 obsolete files)

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.
Blocks: 1352553
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
Attachment #8853534 - Flags: review?(nfroyd)
MozReview-Commit-ID: FFU6WfEqev
Attachment #8853535 - Flags: review?(nfroyd)
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.
Assignee: nobody → michael
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 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.
Attachment #8853534 - Flags: review?(nfroyd) → review+
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.
Attachment #8853535 - Flags: review?(nfroyd) → feedback+
Updated

MozReview-Commit-ID: 5wxbdZwxe7q
Attachment #8853534 - Attachment is obsolete: true
Attachment #8853535 - Attachment is obsolete: true
MozReview-Commit-ID: FFU6WfEqev
Attachment #8854506 - Flags: review?(nfroyd)
Attached file ErrorList.h
This is the generated value of ErrorList.h, for your convenient perusal.
Attachment #8854511 - Attachment mime type: text/x-chdr → text/plain
Attachment #8854512 - Attachment mime type: text/x-chdr → text/plain
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.
Attachment #8854506 - Flags: review?(nfroyd) → review+
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
Attachment #8855045 - Flags: review?(nfitzgerald)
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
Attachment #8855045 - Flags: review?(nfitzgerald) → review+
Pushed by michael@thelayzells.com:
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
(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!
Flags: needinfo?(michael)
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
Pushed by michael@thelayzells.com:
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
Flags: needinfo?(michael)
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.
Flags: needinfo?(michael)
Pushed by michael@thelayzells.com:
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
Relanded
Flags: needinfo?(michael)
This is great. Thank you.
Do we need to touch CLOBBER to prevent other people accidentally add ErrorList.h back again? (e.g. as in bug 1352699)
You need to log in before you can comment on or make changes to this bug.