JIT: fix value-probing schema index collision with handle histogram

[EgorBo]

Both HandleHistogramProbeInstrumentor and ValueInstrumentor were writing to the same BasicBlock::bbHistogramSchemaIndex field. The value instrumentor was essentially copy-pasted from the type histogram one and inherited the same field without adjusting for the fact that a block can be processed by both.

The bug

When a single basic block has both BBF_HAS_HISTOGRAM_PROFILE (e.g. a virtual call) and BBF_HAS_VALUE_PROFILE (a SpanHelpers.Memmove / SpanHelpers.SequenceEqual special intrinsic), schema build runs in this order:

1. HandleHistogramProbeInstrumentor::BuildSchemaElements writes bbHistogramSchemaIndex = N and appends H handle-histogram entries.
2. ValueInstrumentor::BuildSchemaElements overwrites with bbHistogramSchemaIndex = N + H and appends value entries.

Then HandleHistogramProbeInstrumentor::Instrument runs first for the block, reads N + H, and ReadHistogramAndAdvance lands on a ValueHistogramIntCount/ValueHistogramLongCount kind. It returns early without setting typeHistogram/methodHistogram:

• In DEBUG, the assert at https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/fgprofile.cpp#L2077 ("Expected at least one handle histogram when inserting probes") fires, and so does InstrCount() == compHandleHistogramProbeCount later.
• In retail, helperCallNode stays nullptr and gtNewOperNode(GT_COMMA, TYP_REF, helperCallNode, tmpNode2) is built with a nullptr child -- malformed IR, not just a missed probe.

The pattern is common: any method that has a virtual call followed by Span<T>.CopyTo / MemoryExtensions.SequenceEqual / Buffer.Memmove in the same block can hit it once those calls inline down to the recognized SpanHelpers intrinsics.

While at it I also noticed a copy-paste typo in ValueInstrumentor::Prepare that cleared bbCountSchemaIndex instead of its own field. Benign today (count instrumentor's Prepare cleared it earlier), but indicative of the shaky copy-paste.

The fix

Split the per-block schema index into two dedicated fields:

• bbHistogramSchemaIndex -> bbHandleHistogramSchemaIndex (used by HandleHistogramProbeInstrumentor)
• new bbValueHistogramSchemaIndex (used by ValueInstrumentor)

The new field is placed right after the existing union so it consumes the pre-existing 4-byte tail padding before bbTryIndex/bbHndIndex. sizeof(BasicBlock) is unchanged on x64 (280 bytes before and after, verified with a temporary PrintSize<sizeof(BasicBlock)> probe).

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR fixes a correctness issue in CoreCLR JIT PGO instrumentation where handle-histogram probing and value-probing could overwrite the same per-block schema index, causing the handle-histogram instrumentor to read the wrong schema entry for blocks that contain both probe kinds.

Changes:

• Split the shared BasicBlock schema index into bbHandleHistogramSchemaIndex (handle histogram) and a new bbValueHistogramSchemaIndex (value histogram).
• Update HandleHistogramProbeInstrumentor and ValueInstrumentor to write/read their dedicated schema index fields.
• Fix ValueInstrumentor::Prepare to reset its own schema index field (rather than the unrelated count schema index).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/fgprofile.cpp	Switch handle/value instrumentors to use distinct per-block schema index fields and correct the Prepare reset target.
src/coreclr/jit/block.h	Rename the handle-histogram schema index field and add a dedicated value-histogram schema index field on BasicBlock.

[AndyAyersMS]

I wonder if this fixes #123282

[EgorBo]

I wonder if this fixes #123282

I think it's very likely fixing it, yes, at least that assert can fail today because of the issue I'm fixing.
We have very few value probes today so we don't run too often into them

[EgorBo]

@AndyAyersMS No diffs

Slightly more allocated on 32-bit where the new field in BasicBlock slightly increased its layout (no impact on 64bit)

I played locally with multiple value probes and class probes in the same block to test it