clang-cl hits -Wc++11-narrowing in vixl files
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: away, Assigned: away)
References
Details
Attachments
(1 file)
Some of the vixl files fail with -Werror when compiled with clang-cl for aarch64-windows. Some examples:
Instrument-vixl.cpp(422,10): error: case value evaluates to -2147483648, which cannot be narrowed to type 'vixl::Instr' (aka 'unsigned int') [-Wc++11-narrowing]
0:09.11 case STR_w:
Disasm-vixl.cpp(74,10): error: case value evaluates to -1862270976, which cannot be narrowed to type 'vixl::Instr' (aka 'unsigned int') [-Wc++11-narrowing]
0:10.25 case ADD_x_imm: {
Locally I just disable the warning in js-cxxflags.mozbuild. Maybe that's too big a hammer. Do we want something nicer?
Locally I just disable the warning in js-cxxflags.mozbuild. Maybe that's too
big a hammer. Do we want something nicer?
Nathan, do you have an opinion on this?
Comment 2•5 years ago
|
||
Do we patch these files locally? Seems like we should do that, and we should make upstream fix it too because this is a clear thing they're going to fall over on soon enough.
Comment 3•5 years ago
|
||
I think in the short term, disabling the warning is fine. I'm a bit surprised this hasn't shown up in the arm64 ion work; perhaps nobody has been compiling with -Werror there.
(In reply to Jeff Walden [:Waldo] from comment #2)
Do we patch these files locally? Seems like we should do that, and we should make upstream fix it too because this is a clear thing they're going to fall over on soon enough.
ISTR getting mixed signals wrt patching vixl code. I know I directly patched it early in the arm64 windows porting effort, but I thought I remember seeing something about not patching it directly, but patching with full copies of files, or something.
In any event, I also thought I remember seeing something about sstangl doing a fresh vixl import; maybe upstream already has this patched, or we could patch it upstream before the import. sstangl?
In order to get some motion on this I'm going to put up a proposed patch and ask that anyone who objects please tell me specifically what they'd like to see instead.
If vixl had its own moz.build, I'd AllowCompilerWarnings()
and be done with it. As it stands, this patch was the easiest path to getting the werror out of my way.
Updated•5 years ago
|
Comment 6•5 years ago
|
||
In any event, I also thought I remember seeing something about sstangl doing a fresh vixl import.
I got about halfway through an import, at which point we decided to shelve it because the new version wouldn't solve any of our existing problems, and the Qualcomm hardware can't use the fancy new instructions.
maybe upstream already has this patched
Their tree lives here: https://git.linaro.org/arm/vixl.git
It doesn't look like they've patched anything about this.
or we could patch it upstream before the import. sstangl?
Although their code is free software, their development practices are entirely closed. There's no issue tracker or known way to upstream patches to them.
At one point I did contact a VIXL maintainer to ask how I might upstream a patch, and never received a response.
Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b432f26c0035 Disable -Wc++11-narrowing in vixl under clang-cl. r=froydnj
Comment 8•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•