aarch64: ‘JSC::ARM64Assembler::LinkRecord::<unnamed union>::RealTypes::m_compareRegister’ is too small to hold all values of ‘JSC::ARM64Assembler::RegisterID’ {aka ‘enum JSC::ARM64Registers::RegisterID’}

[Michael Catanzaro]

Building 2.24.1 (or maybe 2.24.2, not exactly sure tbh) on aarch64 we have a tremendous warning spam:

[1452/2817] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-02aa2997-2.cpp.o
In file included from ../Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:30,
                 from ../Source/JavaScriptCore/assembler/MacroAssembler.h:46,
                 from ../Source/JavaScriptCore/runtime/BasicBlockLocation.h:30,
                 from ../Source/JavaScriptCore/runtime/ControlFlowProfiler.h:29,
                 from ../Source/JavaScriptCore/runtime/VM.h:35,
                 from ../Source/JavaScriptCore/interpreter/CallFrame.h:30,
                 from ../Source/JavaScriptCore/bytecode/VirtualRegister.h:29,
                 from ../Source/JavaScriptCore/ftl/FTLExitArgumentForOperand.h:31,
                 from ../Source/JavaScriptCore/ftl/FTLExitArgumentForOperand.cpp:27,
                 from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-02aa2997-2.cpp:1:
../Source/JavaScriptCore/assembler/ARM64Assembler.h:465:48: warning: ‘JSC::ARM64Assembler::LinkRecord::<unnamed union>::RealTypes::m_compareRegister’ is too small to hold all values of ‘JSC::ARM64Assembler::RegisterID’ {aka ‘enum JSC::ARM64Registers::RegisterID’}
                 RegisterID m_compareRegister : 6;
                                                ^

[Michael Catanzaro]

This is making it extremely difficult to find build errors.

[Michael Catanzaro]

CCing people who have touched this code before. Looks like this is a sequel to bug #135906. The enum is:

typedef enum : int8_t {
    // Parameter/result registers.
    x0,
    x1,
    x2,
    x3,
    x4,
    x5,
    x6,
    x7,
    // Indirect result location register.
    x8,
    // Temporary registers.
    x9,
    x10,
    x11,
    x12,
    x13,
    x14,
    x15,
    // Intra-procedure-call scratch registers (temporary).
    x16,
    x17,
    // Platform Register (temporary).
    x18,
    // Callee-saved.
    x19,
    x20,
    x21,
    x22,
    x23,
    x24,
    x25,
    x26,
    x27,
    x28,
    // Special.
    fp,
    lr,
    sp,

    ip0 = x16,
    ip1 = x17,
    x29 = fp,
    x30 = lr,
    zr = 0x3f, // <-- problem is here
    InvalidGPRReg = -1,
} RegisterID;

The bitfield is six bits large, so only 32 values are allowed. But 0x3f is 63 decimal. It just doesn't fit. I think actually only 16 nonnegative values are allowed, because it's signed. So yeah, I don't understand how this could work properly without clobbering.

Expanding the bitfield to seven bits is not enough. 8 bits makes the warning go away.

We might not want it to be a bitfield at all at 8 bits, since it's an int8_t enum anyway, but just changing it to eight bits suffices.

[Michael Catanzaro]

Ah, I tried just changing : 6 to : 8, but style checker hates it:

ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:465:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5

Better not use a bitfield, then.

[Michael Catanzaro]

Created attachment 371434 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 371434 [details]
Patch

r=me. If we can shrink LinkRecord, we should represent the register in a bit fields, but in this case, it does not matter much, sizeof(LinkedRecord) does not change.

[WebKit Commit Bot]

Comment on attachment 371434 [details]
Patch

Clearing flags on attachment: 371434

Committed r246151: <https://trac.webkit.org/changeset/246151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.