Fix and refactor basic_bindgen_cflags
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox67 fixed)
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files)
While working on bug 1512504, I realized basic_bindgen_cflags is essentially busted for Windows.
First, because the test for compiler-specific things doesn't test the right thing (compiler_info in os_dict['compiler']
will never match, what is meant is presumably compiler_info.type in os_dict['compiler']
).
Second, because there is no support in there for clang-cl.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
In upcoming changes, we're going to use the normal compiler flags for
bindgen instead of having a separate logic for essentially the same
flags (and there's not much reason not to use the same flags after all).
One hiccup, though is that for some reason, --target=i686-linux-gnu
doesn't work with bindgen while it works with clang in the same setup.
But -m32 does.
Considering -m32 and -m64 are standard flags and that we're using them
in many cases, it doesn't hurt to use them instead of --target with
clang.
While we're doing that, we might as well refactor a little to avoid the
multiple branches handling the use of -m32/-m64, and fix the theoretical
compile-x86_64-with-x86-clang-cl case, as well as the weird check for
aarch64.
And because only two cases actually require -Xclang, only one of which
requires a condition, we just remove the append_flag function; it was
just too confusing, with all the cases that don't need it.
Assignee | ||
Comment 2•5 years ago
|
||
basic_bindgen_cflags's function is to set some flags for use with
rust-bindgen through clang/libclang for C++ code. Part of the flags it
sets are for the C++ standard, and the target.
Unfortunately, some of the logic wrt target-specific flags is currently
broken. It wants to apply per-compiler flags on Windows, but fails to do
so. First, because the condition test is wrong, and second, because it
only cares about msvc and not clang-cl.
OTOH, we already have those flags when the compiler is clang or
clang-cl. And we already have code to get the right flags for a given
compiler. So when the compiler is not clang or clang-cl, we can use that
to get the right flags for the clang we're going to use for bindgen.
Depends on D18316
Assignee | ||
Comment 3•5 years ago
|
||
Note the attached patches unveil a problem with clang 8 (doesn't happen with clang 7), where the build fails during bindgen with the errors like:
09:17:55 ERROR - [style 0.0.1] z:\build\build\src\vs2017_15.9.0p5\VC\include\intrin.h:139:24: error: functions that differ only in their return type cannot be overloaded
09:17:55 INFO - [style 0.0.1] z:\build\build\src\vs2017_15.9.0p5\VC\include\intrin.h:139:24: note: previous implicit declaration is here
09:17:55 ERROR - [style 0.0.1] z:\build\build\src\vs2017_15.9.0p5\VC\include\intrin.h:148:21: error: conflicting types for '_WriteStatusReg'
09:17:55 INFO - [style 0.0.1] z:\build\build\src\vs2017_15.9.0p5\VC\include\intrin.h:148:21: note: '_WriteStatusReg' is a builtin with type 'void (int, int) noexcept'
The content of intrin.h that cause problems are:
__MACHINEARM(int _ReadStatusReg(int))
__MACHINEARM64(__int64 _ReadStatusReg(int))
and
__MACHINEARM(void _WriteStatusReg(int, int, int))
__MACHINEARM64(void _WriteStatusReg(int, __int64))
and what this means is that all of them are expanded when they should not be, causing conflict/overloading errors. And so far, I fail to reproduce when running clang outside bindgen (with the command lines I see the style crate build script invoke), so I'm not sure what bindgen does and why it works with clang 7 but not 8...
Assignee | ||
Comment 4•5 years ago
|
||
This thing is crazy.
Here's how these things are defined in intrin0.h
(slightly simplified):
#define __MACHINEARM __MACHINE
#define __MACHINEARM64 __MACHINE
#define __MACHINE(X) X;
#define __MACHINE_Z() /* Nothing */
#ifndef _M_ARM
#undef __MACHINE_ARM
#define __MACHINE_ARM __MACHINE_Z
#endif
#ifndef _M_ARM64
#undef __MACHINE_ARM64
#define __MACHINE_ARM64 __MACHINE_Z
#endif
The #ifndef __M_ARM
is taken. And the #ifndef __M_ARM64
is not. As expected.
I tried adding something like the following, in multiple places, in intrin0.h
and intrin.h
:
#if __MACHINE_ARM(foo)
#endif
#if __MACHINE_ARM64(foo)
#endif
Because of how preprocessor expansion works, both expressions are errors that are reported, but they are supposed to be different errors if __MACHINE_ARM
and __MACHINE_ARM64
have the right definitions:
The former expands to #if ;
and clang should complain about the token ;
being invalid.
The latter expands to #if
and clang should complain about the missing condition.
In all the places I tried, the expected errors happened. So while the expected errors happened, the expansions still didn't match that. At this point I suspect something changed in the libclang API wrt preprocessing and that throws bindgen off.
Assignee | ||
Comment 5•5 years ago
|
||
I found the culprint: https://reviews.llvm.org/D51204#1381607
Assignee | ||
Comment 6•5 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5)
I found the culprint: https://reviews.llvm.org/D51204#1381607
Assignee | ||
Comment 7•5 years ago
|
||
FWIW, it turns out we've been lucky. The lack of flags doesn't impact the generated bindings.
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/649fc91aa4d3 Use -m32/-m64 in more cases. r=froydnj https://hg.mozilla.org/integration/autoland/rev/ead57b4507ac Fix and refactor basic_bindgen_cflags. r=froydnj
Comment 9•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/649fc91aa4d3
https://hg.mozilla.org/mozilla-central/rev/ead57b4507ac
Description
•