Fix SPMI to handle replays of BBINSTR jit method contexts

[AndyAyersMS]

Move the handling of allocMethodBlockCounts to the method context.
While this data is both an input and an output, we're more interested
in the input side.

We could also treat it as an output, if we say wanted to verify that
the replay jit wrote the same sequence of IL offsets into the BlockCount
records, but that doesn't seem crucial as changes here would also lead
to code diffs.

Fix the code that checks the AddressMap for reloc hints so it doesn't
fail if the exact lookup fails, but instead falls back to a search.
This is needed because the base of the replay count buffer is recorded
as the map key, but the instrumentation probes in jitted code refer
to offsets from this value.

Fixes #37270.

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It seems odd to record both the count "in" argument as well as saving a full buffer of data for the "out" argument, even though that buffer will be empty and newly-allocated memory. In fact, for the purpose of replay, it seems like we don't need to record anything: simply allocate on replay exactly the amount of data required by the replay-time "count" argument to allocMethodBlockCounts. It seems like it doesn't really matter that the count asked for on replay matches the count asked for on collection. If it matters, we could store/retrieve that in the MethodContext, and issue a warning they differ, but still provide the appropriate memory based on the replay count argument.

It seems like the CompileResult side currently has a bug because as far as I can tell, we save off the BlockCounts array when it is allocated, but before it is filled in. It seems like it should be handled more like recAllocGCInfo, where it first caches the function input data, then recAllocGCInfoCapture is called to capture the buffer after it is filled in, at the end of compilation. Doing this for the BlockCounts array would at least capture the IL offsets, though presumably wouldn't capture the counts since those would be filled in during function execution, after compilation is complete.

Comments?

[AndyAyersMS]

It seems like it doesn't really matter that the count asked for on replay matches the count asked for on collection. If

Agree,. This is validation that was there before; I can remove it.

seems like the CompileResult side currently has a bug

The allocated block buffer is no longer part of the compile result, that's the main upshot of this change.

We do need to communicate the buffer address and length to the compile result so that when handling relocs we can properly adjust for the different value we have during replay vs what we saw in record (that is, the address of the buffer appears in codegen, so -- unlike GC info say -- we need to do more work to compensate for it during replay).

seems like it should be handled more like recAllocGCInfo

Right, I alluded to this above ("we could treat it as output"). I don't see doing that as being all that interesting.

[BruceForstall]

I see, so because the address of the recorded table appears in codegen, we need to record that address (and the count) for use with the address map for relocations.

It seems like recAllocMethodBlockCounts can save those, but doesn't need to save the buffer:

   value.pBlockCounts_index =
        AllocMethodBlockCounts->AddBuffer((unsigned char*)*pBlockCounts, count * sizeof(ICorJitInfo::BlockCounts));

and repAllocMethodBlockCounts can just create a new buffer based on the current count, and use the recorded address/count for the address map.

So we wouldn't save the IL offsets in the table in the CR, but that would be ok.

[AndyAyersMS]

Right, no need to save the buffer, as as a MC component, it will always just hold zeros. I'll update.

[AndyAyersMS]

See if you like this new version any better.

[BruceForstall]

LGTM except for one cleanup bit. Good point about associating the buffer properly to get it cleaned up appropriately.

[BruceForstall]

You should delete pBlockCounts_index now (and from the dmp function)

[AndyAyersMS]

X86 tests hit another instance of #11063

Unhandled exception. System.InvalidOperationException: Collection was modified after the enumerator was instantiated.
   at System.Collections.Generic.Stack`1.Enumerator.MoveNext()
   at Xunit.Sdk.DisposalTracker.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\DisposalTracker.cs:line 26

OSX build failed to download the checkout bundle

2020-08-27T08:08:30.6134980Z Download from the specified build: #789827
2020-08-27T08:08:30.6135800Z Download artifact to: /Users/runner/work/1/Checkout_bundle
2020-08-27T08:09:31.3441480Z ##[error]The HTTP request timed out after 00:00:30.
2020-08-27T08:09:31.3947630Z ##[section]Finishing: Download Checkout.bundle