Closed Bug 1524467 Opened 5 years ago Closed 5 years ago

Fix and refactor basic_bindgen_cflags

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
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: nobody → mh+mozilla
Blocks: 1512504
Blocks: 1495669

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.

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

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...

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.

(In reply to Mike Hommey [:glandium] from comment #5)

I found the culprint: https://reviews.llvm.org/D51204#1381607

https://reviews.llvm.org/D57636

Depends on: 1525196

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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1526497
Regressions: 1542622
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: