Closed Bug 1520014 Opened 5 years ago Closed 5 years ago

clang-cl hits -Wc++11-narrowing in vixl files

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
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?

Flags: needinfo?(nfroyd)

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.

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?

Flags: needinfo?(nfroyd) → needinfo?(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.

Attachment #9037609 - Attachment description: Disable -Wc++11-narrowing for clang-cl. → Disable -Wc++11-narrowing in vixl under clang-cl.

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.

Flags: needinfo?(sstangl)
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee: nobody → dmajor
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: