1390691 - Stylo: Struct return type mismatch violates Linux32 ABI

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1390691 - Fix up Servo_StyleSet_Init for Linux 32-bit ABI.

Rust was treating this as returning an `Owned` types which uses a struct, while
C++ saw it as just a pointer.

This disagreement violates the Linux 32-bit ABI, and also the pointer was deemed
to be more correct anyway.

Review commit: https://reviewboard.mozilla.org/r/168916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168916/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1390691 - Fix up Gecko_CalcStyleDifference for Linux 32-bit ABI.

Bindgen bitfield enums don't work as return values with the Linux 32-bit ABI at
the moment because they wrap the value in a struct.

This causes the Rust side to believe the callee expects space for the struct
return value, while C++ believes it's just an integer value.

Review commit: https://reviewboard.mozilla.org/r/168918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168918/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1390691 - Fix up Servo_TakeChangeHint for Linux 32-bit ABI.

Bindgen bitfield enums don't work as return values with the Linux 32-bit ABI at
the moment because they wrap the value in a struct.

This causes the Rust side to believe the caller will pass along space for the
struct return value, while C++ believes it's just an integer value.

Review commit: https://reviewboard.mozilla.org/r/168920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168920/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1390691 - Fix up Servo_StyleSet_MediumFeaturesChanged for Linux 32-bit ABI.

Bindgen bitfield enums don't work as return values with the Linux 32-bit ABI at
the moment because they wrap the value in a struct.

This causes the Rust side to believe the caller will pass along space for the
struct return value, while C++ believes it's just an integer value.

Review commit: https://reviewboard.mozilla.org/r/168922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168922/

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8897648 [details]
Bug 1390691 - Fix up Servo_StyleSet_Init for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168916/#review174272

Comment 6

[Manish Goregaokar [:manishearth]]

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

:(

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8897650 [details]
Bug 1390691 - Fix up Servo_TakeChangeHint for Linux 32-bit ABI.

https://reviewboard.mozilla.org/r/168920/#review174276

Comment 8

[Manish Goregaokar [:manishearth]]

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`

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8897650 [details]
Bug 1390691 - Fix up Servo_TakeChangeHint for Linux 32-bit ABI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168920/diff/1-2/

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8897651 [details]
Bug 1390691 - Fix up Servo_StyleSet_MediumFeaturesChanged for Linux 32-bit ABI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168922/diff/1-2/

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a190e87a3a3dd0faa1f7ed789683e636247c4933

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://github.com/servo/servo/pull/18100

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8897648 [details]
Bug 1390691 - Fix up Servo_StyleSet_Init for Linux 32-bit ABI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168916/diff/1-2/

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8897649 [details]
Bug 1390691 - Fix up Gecko_CalcStyleDifference for Linux 32-bit ABI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168918/diff/1-2/

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8897650 [details]
Bug 1390691 - Fix up Servo_TakeChangeHint for Linux 32-bit ABI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168920/diff/2-3/

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8897651 [details]
Bug 1390691 - Fix up Servo_StyleSet_MediumFeaturesChanged for Linux 32-bit ABI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168922/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c30ae9302d7
https://hg.mozilla.org/mozilla-central/rev/fa23649e8468
https://hg.mozilla.org/mozilla-central/rev/6451e509bea1
https://hg.mozilla.org/mozilla-central/rev/5ffa205aea21