[gardening] assembler_x64.cc: 2774: error: expected: Utils::IsInt(32, disp)

[dcharkes]

LoadIndexedInstr::EmitNativeCode on x64 can hit an assert.

../../runtime/vm/compiler/assembler/assembler_x64.cc: 2774: error: expected: Utils::IsInt(32, disp)
version=3.3.0-edge (main) (Unknown timestamp) on "linux_x64"
pid=18806, thread=18806, isolate_group=isolate(0x5632001668b0), isolate=(nil)((nil))
os=linux, arch=x64, comp=no, sim=no
isolate_instructions=0, vm_instructions=0
fp=7ffd97664f10, sp=7ffd97664dd8, pc=5631ff837d4c
  pc 0x00005631ff837d4c fp 0x00007ffd97664f10 dart::Profiler::DumpStackTrace+0x7c
  pc 0x00005631ff578cb4 fp 0x00007ffd97664ff0 dart::Assert::Fail+0x84
  pc 0x00005631ffc1ed37 fp 0x00007ffd97665030 dart::compiler::Assembler::ElementAddressForIntIndex+0xa7
  pc 0x00005631ffcb9e9d fp 0x00007ffd976650a0 dart::LoadIndexedInstr::EmitNativeCode+0x1ad
  pc 0x00005631ffc463d9 fp 0x00007ffd97665200 dart::FlowGraphCompiler::VisitBlocks+0x429
  pc 0x00005631ffc45ede fp 0x00007ffd97665230 dart::FlowGraphCompiler::CompileGraph+0x2e
  pc 0x00005631ffd414b0 fp 0x00007ffd97665240 dart::CompilerPass_GenerateCode::DoBody+0x10
  pc 0x00005631ffd3f826 fp 0x00007ffd97665310 dart::CompilerPass::Run+0x126
  pc 0x00005631ffbeb9a3 fp 0x00007ffd976659f0 dart::PrecompileParsedFunctionHelper::Compile+0x6f3
  pc 0x00005631ffbec554 fp 0x00007ffd976662b0 dart::PrecompileFunctionHelper+0x394
  pc 0x00005631ffbe6bfc fp 0x00007ffd976663d0 dart::Precompiler::CompileFunction+0x1ac
  pc 0x00005631ffbe55de fp 0x00007ffd97666470 dart::Precompiler::ProcessFunction+0x1ae
  pc 0x00005631ffbdfbf4 fp 0x00007ffd976664c0 dart::Precompiler::Iterate+0x94
  pc 0x00005631ffbdc4bc fp 0x00007ffd97666dd0 dart::Precompiler::DoCompileAll+0x192c
  pc 0x00005631ffbdaae6 fp 0x00007ffd97667260 dart::Precompiler::CompileAll+0xb6
  pc 0x00005631ffe1b3ce fp 0x00007ffd976673c0 Dart_Precompile+0x1be
  pc 0x00005631ff54ab22 fp 0x00007ffd97667550 dart::bin::main+0x8f2
-- End of DumpStackTrace
=== Crash occurred when compiling file:///b/s/w/it8xfz4hkl/dart_fuzzRUBMXP/fuzz.dart_::_fooE4|foo4_Extension2 in AOT mode in GenerateCode pass
=== When compiling block B41[join]:468 pred(B24, B42) {
      v99 <- phi(v428 T{_Smi}, v137) alive [-9223372036854775808, 9223372036854775807] int64 T{int}
}
=== When compiling instruction v536 <- LoadIndexed(v258 T{_Int32x4List}, v816 T{_Smi}) int32x4

-- BEGIN REPRODUCE  --

DART SDK REVISION: 

dart runtime/tools/dartfuzz/dartfuzz.dart --no-fp --no-ffi --flat --seed 1258834069 fuzz.dart

-- RUN 1 --

out/DebugSIMARM64/dart --use-slow-path --old_gen_heap_size=128 /b/s/w/it8xfz4hkl/dart_fuzzRUBMXP/fuzz.dart

-- RUN 2 --

DART_CONFIGURATION='DebugX64' DART_VM_FLAGS='--enable-asserts' pkg/vm/tool/precompiler2 --use-slow-path --deterministic fuzz.dart snapshot
pkg/vm/tool/dart_precompiled_runtime2 snapshot

-- END REPRODUCE  --

full log: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8760042587664820481/+/u/collect_shards/make_a_fuzz_shard_12/task_stdout_stderr:_make_a_fuzz_shard_12

[dcharkes]

Repro's locally:

dart runtime/tools/dartfuzz/dartfuzz.dart --no-fp --no-ffi --flat --seed 1258834069 fuzz.dart
out/DebugX64/dart-sdk/bin/dart compile aot-snapshot fuzz.dart

[dcharkes]

I believe we are able to generate an indexing instruction that is out of the 32 bit bound.

cc @sstrickl

[sstrickl]

Just adding investigation notes here:

This should be coming from the following code in fuzz.dart:

Int32x4List var25 = Int32x4List(22);
...
extension fooE4 on Int64List {
 Int32x4? foo4_Extension2(Map<String, String>? par1, WeakReference<bool>? par2, int par3){
  ... var25[(-((-33 >>> 8)))] ...
 }
}

and

-(-33 >>> 8) == -72057594037927935

The relevant code in the IL is:

   ....
    v702 <- UnboxedConstant(#-72057594037927935) [-72057594037927935, -72057594037927935] int64
    ...
    v816 <- Constant(#-1152921504606846960) [-1152921504606846960, -1152921504606846960] T{_Smi}
...
548:     GenericCheckBound:14(v779 T{_Smi}, v702) [-4611686018427387904, 4611686018427387903] int64
550:     v536 <- LoadIndexed(v258 T{_Int32x4List}, v816 T{_Smi}) int32x4

To give hex versions to make it clearer,

v702 = 0xff00 0000 0000 0001 == -(-33 >>> 8)
v816 = 0xf000 0000 0000 0010 == v702 << 4 == v702 * Int32x4List.elementSizeInBytes

Note that the GenericCheckBound is guaranteed to fail, since v702 is a negative 64-bit value and thus, when compared using unsigned >= to the length of var25, which is guaranteed to be a positive value that fits in a Smi, the comparison will be true and cause the control flow to go to the slow path for raising the appropriate error.

That is, all the code dominated by the GenericCheckBound instruction, including the LoadIndexed here, is dead code and could be removed.

[sstrickl]

The primary issue here is that Assembler::ElementAddressForIntIndex doesn't have the ability to tell the caller 'Sorry, the generated displacement can't fit in a single Assembler::Address value', which would then allow the caller to fix this by putting the offset in a register and then use the register version instead.

Well, that's not entirely true. We could change Assembler::ElementAddressForIntIndex on X64 to see if the displacement doesn't fit, and if it doesn't and the array register isn't TMP, we could just put the offset into TMP and then return the result of Address::ElementAddressForRegIndex using TMP as the index register. It's a little iffy because we're possibly generating an assembler instruction in Assembler::ElementAddressForIntIndex and it's also the case that the TMP register might have already been live across a LoadIndexed instruction, but eh, that's unlikely and it's probably the simplest fix to get things working without causing too much code churn.

(Now I want to have a way to mark "assembler-internal temp register X is currently live" (where X is TMP or TMP2) in the Assembler so that we can have checks that assembler (macro)instructions that use X internally can throw appropriate errors if their expectation that X is not currently live is broken.)

[sstrickl]

Actually, other architectures like ARM already have ElementAddressForIntIndex do this, so nevermind, this is perfectly cromulent in our current style.

(Just means we have to change this from a static method to an instance method on X64, but the fact that it's an instance method on some architectures and a static method on others is already ick.)

[sstrickl]

After some more investigation, turns out that the actual issue is that CanBeImmediateIndex, a local function used on all architectures to determine whether the index location for a LoadIndexed or StoreIndexed instruction should be a constant or a register, assumed that the scaling factor for the index could be calculated from the cid, but those instructions have an index_scale field that can differ from the element size for the cid.

Thus the real fix is to make sure that CanBeImmediateIndex calculates the same displacement as ElementAddressForIntIndex by passing it the same field values from the instruction, so the instruction doesn't try and pass a constant location to EmitNativeCode that can't actually fit in the immediate field of a single instruction.