Closed Bug 1319968 Opened 8 years ago Closed 7 years ago

Non-stylo builds broken on stylo repository

Categories

(Firefox Build System :: General, defect, P2)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: gps, Unassigned)

References

Details

Non-stylo builds are broken on the stylo repository. They fail during a link due to duplicate symbols related to Servo/Stylo.

The reason for this bustage is likely the following changes to the rust library in the stylo repo:

diff --git a/toolkit/library/rust/shared/Cargo.toml b/toolkit/library/rust/shared/Cargo.toml
--- a/toolkit/library/rust/shared/Cargo.toml
+++ b/toolkit/library/rust/shared/Cargo.toml
@@ -6,6 +6,7 @@ license = "MPL-2.0"
 description = "Shared Rust code for libxul"

 [dependencies]
+geckoservo = { path = "../../../../servo/ports/geckolib" }
 mp4parse_capi = { path = "../../../../media/libstagefright/binding/mp4parse_capi" }
 nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rust_url_capi = { path = "../../../../netwerk/base/rust-url-capi" }
diff --git a/toolkit/library/rust/shared/lib.rs b/toolkit/library/rust/shared/lib.rs
--- a/toolkit/library/rust/shared/lib.rs
+++ b/toolkit/library/rust/shared/lib.rs
@@ -2,6 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.

+extern crate geckoservo;
 extern crate mp4parse_capi;
 extern crate nsstring;
 extern crate rust_url_capi;


The inclusion of geckoservo is unconditional and introduces duplicate symbols when not building with stylo.

I tried to fix this with the following patch:

diff --git a/toolkit/library/rust/shared/Cargo.toml b/toolkit/library/rust/shared/Cargo.toml
--- a/toolkit/library/rust/shared/Cargo.toml
+++ b/toolkit/library/rust/shared/Cargo.toml
@@ -5,8 +5,11 @@ authors = ["nobody@mozilla.org"]
 license = "MPL-2.0"
 description = "Shared Rust code for libxul"

+[features]
+servo = ["geckoservo"]
+
 [dependencies]
-geckoservo = { path = "../../../../servo/ports/geckolib" }
+geckoservo = { path = "../../../../servo/ports/geckolib", optional = true }
 mp4parse_capi = { path = "../../../../media/libstagefright/binding/mp4parse_capi" }
 nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rust_url_capi = { path = "../../../../netwerk/base/rust-url-capi" }


Combined with a hack to pass `--features servo` to cargo in the build system. However, that errored about wanting to update Cargo.lock, which it couldn't do because we pass --frozen into Cargo. If you try to run `mach vendor rust` or `cargo update` manually, it deletes a bunch of files from third_party/rust and removes the corresponding entries from Cargo.lock. It certainly appears like cargo isn't picking up that optional dependency on geckoservo and all the other transitive dependencies. I'm not sure what magic combination of Cargo configs and invocations are necessary to support both a build with the geckoservo dependency and without.
bholley was kind enough to point out some code in StyloBindings.cpp that pulls in the duplicate symbols:

#ifndef MOZ_STYLO
#define SERVO_BINDING_FUNC(name_, return_, ...)                               \
  return_ name_(__VA_ARGS__) {                                                \
    MOZ_CRASH("stylo: shouldn't be calling " #name_ "in a non-stylo build");  \
  }
#include "ServoBindingList.h"
#undef SERVO_BINDING_FUNC
#endif

If you remove this code, the linker is happy it isn't seeing duplicate symbols and it proceeds to the next error:

 ../../build/unix/gold/ld: error: /home/gps/src/firefox/obj-x86_64-pc-linux-gnu/toolkit/library/../../layout/style/Unified_cpp_layout_style3.o: requires dynamic R_X86_64_PC32 reloc against 'Servo_GetStyleVariables' which may overflow at runtime; recompile with -fPIC
 4:48.30 ../../build/unix/gold/ld: error: read-only segment has dynamic relocations
 4:48.30 /home/gps/src/firefox/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:79: error: undefined reference to 'Servo_GetStyleVariables'
 4:48.30 /home/gps/src/firefox/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:79: error: undefined reference to 'Servo_GetStyleVariables'

I imagine with a little iteration I could remove all the references to offending symbols and coerce this thing into linking.

However, even if the symbols are sorted out, there's still the problem that toolkit/library/rust/shared/Cargo.toml from the stylo repository adds "geckoservo" as a dependency. AFAICT this essentially makes cargo build servo and all its dependencies. However, my contention is that since these dependencies aren't necessary for Gecko builds without Stylo enabled, they shouldn't be built at all because building them is just wasting time. I'm not sure how to make the geckoservo dependency conditional while making `cargo vendor` and various Cargo.lock files happy.
That error suggests visibility discrepancy. Where is Servo_GetStyleVariables defined, and where is it declared?
The declaration is at the bottom of layout/style/ServoBindingList.h and the definition is at the bottom of layout/style/ServoBindings.cpp.
Since this is a special case, where we don't have a Servo-defined function of this name (but we have a C++ function named Servo_GetStyleVariables just so our C++ macros that do things based on struct names finds something it expects), maybe we should instead of #ifdefing the guts of the function, so that it MOZ_CRASHes if MOZ_STYLO is not defined.
How are these functions defined and declared when stylo is enabled?
For all the other Servo_GetStyleXXX functions:  The declarations should be the same regardless of whether MOZ_STYLO is defined.  In the MOZ_STYLO case, the function is defined on the Servo side and not on the C++ side.  In the non-MOZ_STYLO case, the function is defined on the C++ side.

For Servo_GetStyleVariables specifically, there is no Servo definition of this function.  In the MOZ_STYLO case, we have the C++ definition (a stop-gap implementation of this function since Servo doesn't have it yet).  In the non-MOZ_STYLO case, there shouldn't be any calls to this function; it'll be declared and not defined.  (I guess we could #ifdef MOZ_STYLO the declarations so we don't accidentally have any references to them.)
As far as the compiler is concerned, Servo_GetStyleVariables is called (twice) from the following function in non-stylo builds:

  nsChangeHint nsStyleContext::CalcStyleDifferenceInternal<FakeStyleContext>(FakeStyleContext*, nsChangeHint, unsigned int*, unsigned int*)
Interestingly, I don't reproduce the build failure on the current tip of the stylo repository, and if I do what's suggested in comment 4, I do get a build error because the function is redefined:

 0:05.08 /mnt/stylo/layout/style/ServoBindings.cpp: In function �const nsStyleVariables* Servo_GetStyleVariables(ServoComputedValuesBorrowed)�:
 0:05.08 /mnt/stylo/layout/style/ServoBindings.cpp:1103:1: error: redefinition of �const nsStyleVariables* Servo_GetStyleVariables(ServoComputedValuesBorrowed)�
 0:05.08  Servo_GetStyleVariables(ServoComputedValuesBorrowed aComputedValues)
 0:05.08  ^
 0:05.08 In file included from /mnt/stylo/obj-x86_64-pc-linux-gnu/layout/style/Unified_cpp_layout_style1.cpp:65:0:
 0:05.08 /mnt/stylo/layout/style/ServoBindingList.h:171:22: note: �const nsStyleVariables* Servo_GetStyleVariables(ServoComputedValuesBorrowedOrNull)� previously defined here
 0:05.08    SERVO_BINDING_FUNC(Servo_GetStyle##name, const nsStyle##name*,  \
 0:05.08                       ^
 0:05.08 /mnt/stylo/layout/style/ServoBindings.cpp:1095:11: note: in definition of macro �SERVO_BINDING_FUNC�
 0:05.09    return_ name_(__VA_ARGS__) {                                                \
 0:05.09            ^
 0:05.09 /mnt/stylo/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:18:3: note: in expansion of macro �STYLE_STRUCT�
 0:05.09    STYLE_STRUCT(name, checkdata_cb)
 0:05.09    ^
 0:05.09 /mnt/stylo/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:79:1: note: in expansion of macro �STYLE_STRUCT_INHERITE �
 0:05.09  STYLE_STRUCT_INHERITED(Variables, CheckVariablesCallback)
 0:05.09  ^
 0:05.09 In file included from /mnt/stylo/obj-x86_64-pc-linux-gnu/layout/style/Unified_cpp_layout_style1.cpp:65:0:
 0:05.09 /mnt/stylo/layout/style/ServoBindings.cpp:1114:1: error: expected �;� before �}� token
 0:05.09  }
Ah, I see we are defining it once due to the SERVO_BINDING_FUNC entry in ServoBindingList.h, and a second time explicitly in ServoBindings.cpp.

It might be easiest just to define this stop-gap Servo_GetStyleVariables function on the Servo side, and then we don't need to treat Variables any differently from the other style struct, in terms of the macro-generated crashing stub functions.
Depends on: 1319982
Blocks: 1321035
Blocks: stylo-central
No longer blocks: stylo
Priority: -- → P2
Cameron, are the non-stylo builds still broken in the incubator/stylo repo? Now that we are very close to moving development from incubator/stylo to mozilla-central, we probably don't care about this..
Flags: needinfo?(cam)
They are no longer broken as of cameron's last sync, and we also probably don't need this anymore.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(cam)
Resolution: --- → WORKSFORME
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.