Stylo: Struct return type mismatch violates Linux32 ABI

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
There are several places in the Stylo FFI boundary where one side thinks it's returning a struct and other side sees something else.

With the Linux32 ABI, this causes havoc because returned structs are expected to allocated on the caller's stack and passed as a pointer before all the real arguments.  If one side disagrees, then arguments will all be off by one.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8897648 [details]
Bug 1390691 - Fix up Servo_StyleSet_Init for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168916/#review174272
Attachment #8897648 - Flags: review?(manishearth) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8897649 [details]
Bug 1390691 - Fix up Gecko_CalcStyleDifference for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168918/#review174274

::: layout/style/ServoBindings.h:399
(Diff revision 1)
>  // Not if we do them in Gecko...
>  nsStyleContext* Gecko_GetStyleContext(RawGeckoElementBorrowed element,
>                                        nsIAtom* aPseudoTagOrNull);
>  mozilla::CSSPseudoElementType Gecko_GetImplementedPseudo(RawGeckoElementBorrowed element);
> -nsChangeHint Gecko_CalcStyleDifference(ServoStyleContextBorrowed old_style,
> +// We'd like to return `nsChangeHint` here, but bindgen bitfield enums don't
> +// work as return values with the Linux 32-bit ABI at the moment because

:(
Attachment #8897649 - Flags: review?(manishearth) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8897650 [details]
Bug 1390691 - Fix up Servo_TakeChangeHint for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168920/#review174276
Attachment #8897650 - Flags: review?(manishearth) → review+

Comment 8

2 years ago
mozreview-review
Comment on attachment 8897651 [details]
Bug 1390691 - Fix up Servo_StyleSet_MediumFeaturesChanged for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168922/#review174278

::: servo/ports/geckolib/glue.rs:903
(Diff revision 1)
>  
>  #[no_mangle]
>  pub extern "C" fn Servo_StyleSet_MediumFeaturesChanged(
>      raw_data: RawServoStyleSetBorrowed,
>      viewport_units_used: *mut bool,
> -) -> OriginFlags {
> +) -> u8 {

could you change the type on the gecko side too? otherwise this will fail `./mach test-stylo`
Attachment #8897651 - Flags: review?(manishearth) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8897651 [details]
Bug 1390691 - Fix up Servo_StyleSet_MediumFeaturesChanged for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168922/#review174278

> could you change the type on the gecko side too? otherwise this will fail `./mach test-stylo`

Good catch, I fixed these up and verified the tests pass (at least as well they did before on Linux32).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7c30ae9302d7
Fix up Servo_StyleSet_Init for Linux 32-bit ABI. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/fa23649e8468
Fix up Gecko_CalcStyleDifference for Linux 32-bit ABI. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/6451e509bea1
Fix up Servo_TakeChangeHint for Linux 32-bit ABI. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/5ffa205aea21
Fix up Servo_StyleSet_MediumFeaturesChanged for Linux 32-bit ABI. r=manishearth
You need to log in before you can comment on or make changes to this bug.