1286089 - Make stylo build with Visual Studio

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

With servo/servo#12377[1] fixed, geckolib of servo can be compiled with msvc backend. However, the flags passed to the linker on Gecko side do not work well with Visual Studio.

I'll investigate how to fix it.

(It is possible that the bindgen doesn't work well with MSVC either... We'll see later.)


[1] https://github.com/servo/servo/pull/12377

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

So yes, bindgen doesn't work well with MSVC. I have a patch to make Gecko find the geckolib, but the link still fails with errors like:
> geckoservo.lib(geckoservo.0.o) : error LNK2019: unresolved external symbol __imp__ZN9nsGkAtoms5serifE referenced in function _ZN86_$LT$properties..GeckoFont$u20$as$u20$style..properties..style_struct_traits..Font$GT$15set_font_family17h1eeec00d565db32aE

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Looks like it isn't something generated by bindgen.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yep, so I know what this is.

So basically when Bobby introduced support for the atom macro in Servo[1], he only mangled names correctly for OSX/Linux. We kind of knew this moment was coming, but decided to hold off running bindgen on the atoms for now[2].

The correct thing would be doing something like the thing I commented on that issue[3]. I can probably do that tomorrow (though we'd need different bindings for win32 and Linux/OSX), so they need to be generated carefully and probably can't be updated often. 

Xidorn, if you have bindgen working on Windows, could you just gist/pastebin the output of the command in[3] (or similar? maybe some bindgen option has changed), so I can land that change tomorrow? Hopefully clang does the correct mangling, otherwise I can try to do it tweaking bindgen, I know Vlad had some problems with this.

As a temporary solution to keep moving, changing the python script would work too.

[1]: https://github.com/servo/servo/pull/11242
[2]: https://github.com/servo/servo/pull/11242#issuecomment-220118034
[3]: https://github.com/servo/servo/pull/11242#discussion_r63677306

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

No, I haven't had bindgen work on Windows, but it seems the linker doesn't raise any issue with those generated by bindgen, so I'm looking at structs_*.rs and want to figure out whether there is any useful clue.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So, the only site we pull-in those symbols is from https://github.com/servo/servo/blob/master/ports/geckolib/string_cache/atom_macro.rs

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I'm now confused. When I change the python script to use link_name in form "?atom_name@nsGkAtoms@@2PEAVnsIAtom@@EA", then linking would succeed with lots of warnings LNK4217 and LNK4049 (which warns that we use dllimport on a locally defined symbol). But if that is the way MSVC mangles name, why there isn't any linking error for the style structs?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

We don't pull that many symbols from the structs file, and I don't think we use any of them to be fair (so they might just be thrown away by rustc).

The only important bits in the struct_xxx.rs files are the struct layout definitions, all the calls are done via ffi functions (bindings.rs) that have defined (C like) symbols.

So... Were you able to build? If so that's awesome! :)

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> We don't pull that many symbols from the structs file, and I don't think we
> use any of them to be fair (so they might just be thrown away by rustc).

That sounds reasonable...

> The only important bits in the struct_xxx.rs files are the struct layout
> definitions, all the calls are done via ffi functions (bindings.rs) that
> have defined (C like) symbols.

Hmmm, I guess there could be some issue on that... I believe MSVC has some different alignment algorithm.

> So... Were you able to build? If so that's awesome! :)

Yeah, it builds! But I'd like to know if it is possible to get rid of the warnings... And I also want to test whether it works for 32bit.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> Hmmm, I guess there could be some issue on that... I believe MSVC has some
> different alignment algorithm.

Hmm... So bindgen has actually only be used on windows for Servo's SpiderMonkey bindings[1] with GCC, but I think Vlad had a working version of them for MSVC?[1].

SM has bindings per platform, our manual binding check-in stuff should be solved after the repo integration thing, but... There are three possible outcomes here:

 1. Everything works out of the box. I *hope* to be in this situation, in the sense that all the style structs are simple enough (no multiple inheritance, no virtual stuff) in order for the layout to be C-like. Should be easy enough to do a program compiled with MSVC that dumped all the sizes of the structs, to verify at least the sizes match the expected ones. Doing the field offset would be valuable too, but more tedious.

 2. libclang works differently on windows (detects the MSVC target? I think they have one). Worst case we'll need two set of bindings per platform.

 3. Worst case: MSVC really alters our struct layout weirdly. I *hope* this is not the case at least for the types we care about. If it is, we could try to reorder the struct fields to appease the fury of MSVC, or (I hope we/I don't have to do this) reverse-engineer MSVC's struct layout algo.

I'm going to ni Vlad just to know if he tested the bindgen-generated SpiderMonkey bindings for MSVC, and if that worked fine.

[1]: https://github.com/servo/rust-mozjs/
[2]: https://github.com/servo/rust-mozjs/pull/270

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

I guess it would be great if we can generate a cpp file having bunch of static_asserts which assert that the layout C++ compilers generate is identical to what we expect Rust would give us. C++ has sizeof, alignof, and offsetof, so we can test everything at compile time.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

If I read rust-lang/rust#27438 [1] correctly, rustc always adds dllimport for extern constants, which means it is currently not able to fix warning LNK4217 and LNK4049. So it seems we have to live with them for a while.

[1] https://github.com/rust-lang/rust/issues/27438

Comment 12

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> I guess it would be great if we can generate a cpp file having bunch of
> static_asserts which assert that the layout C++ compilers generate is
> identical to what we expect Rust would give us. C++ has sizeof, alignof, and
> offsetof, so we can test everything at compile time.

Yes! This is exactly what I was imagining we'd want to do as soon as we started using MSVC. Should be straightforward enough to do - at the moment it will be rather inconvenient to include that cpp file in the gecko build, but we can do it manually for now and then hook it up permanently once the repo integration is done.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

So I currently have this patch on Servo: https://github.com/servo/servo/compare/master...upsuper:msvc-atoms

I'm not sure whether we want to use this hack... Or do something in bindgen to make it support MSVC and use that to generate this?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> So I currently have this patch on Servo:
> https://github.com/servo/servo/compare/master...upsuper:msvc-atoms
> 
> I'm not sure whether we want to use this hack... Or do something in bindgen
> to make it support MSVC and use that to generate this?

Ideally the latter, I can try to get a windows vm and setup all the msvc + clang stuff so bindgen works. Until then I guess something similar to that can work.

Comment 15

[Bobby Holley (:bholley)]

This is a strict improvement over the status quo, so let's just get it reviewed and landed for now. If we want to do something fancy with bindgen we can do it later.

Comment 16

[Bobby Holley (:bholley)]

That is to say, doing the atom linkage "right" isn't as much of a priority as the other things we could be doing right now, and will need to be redone anyway when we switch to bindgen-at-buildtime.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

I'm not very optimistic about bindgen-at-buildtim, because bindgen requires LLVM, and moving from that is probably non-trivial... Do we want to require LLVM as a build requirement everywhere, even if the compiler we use is GCC or MSVC?

Comment 18

[Gregory Szorc [:gps]]

Having LLVM/Clang everywhere isn't horrible: it makes it easier to standardize around LLVM/Clang tooling for things like static analyzers. I'd prefer for more of these things to "just work" for local builds and requiring LLVM/Clang solves that problem. Of course, you have problems where you may have to build things twice if you want to use the shipping platform and the platform the tools run on. Oh, how I wish we could ship from Clang on all platforms.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> I'm not very optimistic about bindgen-at-buildtim, because bindgen requires
> LLVM, and moving from that is probably non-trivial... Do we want to require
> LLVM as a build requirement everywhere, even if the compiler we use is GCC
> or MSVC?

Eventually that is the plan - but it's kinda off-topic for this bug, so let's focus more on the issue here.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1286089 - Ensure servo can be linked properly for different compilers.

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

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8770391 [details]
Bug 1286089 - Ensure servo can be linked properly for different compilers.

https://reviewboard.mozilla.org/r/63806/#review61470

I'd rather not pile up hacks on top of hacks. Let's fix this more "properly".

Comment 22

[Mike Hommey [:glandium]]

So, I did in the build system frontend what I thought would help, but it doesn't. That doesn't make the current patch more desirable, though. Please just create the file path in old-configure, depending whether building with msvc or not (e.g. test -n "$_WIN32_MSVC").

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

I actually don't understand why the current patch is not desirable. I didn't consider it as a hack: gcc and clang use a different syntax than msvc for library with path, so I generate different parameter for them in the frontend if it is not just a name.

What's wrong with this idea? What do you think would be more "properly"?

Comment 24

[Mike Hommey [:glandium]]

The situation with OS_LIBS is that it's not as convenient as USE_LIBS, and not meant to be. And it's better the frontend doesn't try to be smart about what's given in OS_LIBS, because it can contain all kinds of things. The -l thing is already a hack of some sort. There's also the test for a $ being in there. I don't want pile up on that.

> What do you think would be more "properly"?

cf. comment 22.

Something like

if test -n "$_WIN32_MSVC"; then
    MOZ_SERVO_LIBS="${SERVO_TARGET_DIR}/geckoservo.lib"
else
    MOZ_SERVO_LIBS="-L${SERVO_TARGET_DIR} -lgeckoservo"
fi

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8770391 [details]
Bug 1286089 - Ensure servo can be linked properly for different compilers.

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

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8770391 [details]
Bug 1286089 - Ensure servo can be linked properly for different compilers.

https://reviewboard.mozilla.org/r/63806/#review62364

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/875e441f8b80
Ensure servo can be linked properly for different compilers. r=glandium

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/875e441f8b80