Closed Bug 1390214 Opened 2 years ago Closed 2 years ago

Encoding-x86-shared.h:318:12: warning: comparison of two values with different enumeration types in switch statement

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: dholbert, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When building mozilla-central with clang trunk (version 6.0, locally built from revision 310772), I get the following build warning:

===========
js/src/jit/x86-shared/Encoding-x86-shared.h:318:12: warning: comparison of two values with different enumeration types in switch statement ('js::jit::X86Encoding::TwoByteOpcodeID' and 'js::jit::X86Encoding::ThreeByteOpcodeID') [-Wenum-compare-switch]

       case OP3_PEXTRD_EdVdqIb:
            ^~~~~~~~~~~~~~~~~~
===========

I think this might be a new warning in clang? Anyway, it looks kind of legit... Seems like a very odd use-case to have case statements for different enum types in the same switch block ( TwoByteOpcodeID vs ThreeByteOpcodeID ).  Here's the code in question, with a comment added on the problematic line:

> // Test whether the given opcode should be printed with its operands reversed.
> inline bool IsXMMReversedOperands(TwoByteOpcodeID opcode)
> {
>     switch (opcode) {
>       case OP2_MOVSD_WsdVsd: // also OP2_MOVPS_WpsVps
>       case OP2_MOVAPS_WsdVsd:
>       case OP2_MOVDQ_WdqVdq:
>       case OP3_PEXTRD_EdVdqIb: // *****CLANG WARNS FOR THIS ONE*****
>         return true;
>       default:
>         break;
>     }
>     return false;
> }

This definitely looks iffy, even beyond the switch statement warning, because the type of the function-param |opcode| is TwoByteOpcodeID, and OP3_PEXTRD_EdVdqIb is *not* that type.

Also, as a side note -- there *are* a couple of TwoByteOpcodeID enum-values with the same numeric value as "OP3_PEXTRD_EdVdqIb", FWIW -- not sure if that's problematic or not from the perspective of this mixed-enum-type switch statement. Specifically, there are:
> enum TwoByteOpcodeID {
>     [...]
>     OP2_MOVLHPS_VqUq    = 0x16,
>     OP2_MOVSHDUP_VpsWps = 0x16,

...vs.:

> enum ThreeByteOpcodeID {
>     [...]
>     OP3_PEXTRD_EdVdqIb  = 0x16,

So we might be accidentally returning "True" for those OP2_*** opcodes here.

HISTORICAL NOTE:
This line was added here:
https://hg.mozilla.org/mozilla-central/rev/736d53322a1d91210f65e66e8d5254ddd791a370#l3.33
as part of bug 1115752. Marking as dependency of that bug.
Flags: needinfo?(sunfish)
IsXMMReversedOperands isn't used for 3-byte opcode encodings, so this case can be removed.
Assignee: nobody → sunfish
Status: NEW → ASSIGNED
Flags: needinfo?(sunfish)
Duplicate of this bug: 1395131
Comment on attachment 8902972 [details] [diff] [review]
threebyte-reversed-operands.patch

IsXMMReversedOperands isn't used for 3-byte opcodes, so this case can be removed.
Attachment #8902972 - Flags: review?(jdemooij)
Attachment #8902972 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5316b09380776ef8cc826dc6cc326f92f49d4965
Bug 1390214 - IonMonkey: Don't test for a 3-byte opcode in a 2-byte opcode predicate r=jandem
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5316b0938077
IonMonkey: Don't test for a 3-byte opcode in a 2-byte opcode predicate r=jandem
Dan, I took the liberty to land it for you (with the ownership obviously) as it is breaking some CIs.
https://hg.mozilla.org/mozilla-central/rev/5316b0938077
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.