Safer stackallocs

[EgorBo]

First batch of changes around various stackalloc expressions in BCL.

The rules are:

1. Make sure unbound stackallocs have upper (and lower!) bound checks. Add (uint) casts to handle negative/overflows as well (I've checked all places that it's only applied to int, no long, etc.). In many places it's redundant, but it shouldn't affect the performance and reduce number of false-positives reported by automated tooling.
   Also, add Debug.Assert to make it more clear.
   2) For patterns like len > CONST : stackalloc T[CONST] : new T[len] we change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 1024 bytes in order to consume less stack. It shouldn't hurt performance since our libs are compiled with SkipLocalsInit (although, a small overhead still there, so we leave it as is for small const sizes).
   UPD: although, maybe we should do it only when it's saved to Span to make sure nobody relies on some minimal size..
2. Flag suspiciously large allocations
3. Change unmanaged pointers to Spans on the left side of the stackalloc expression (this PR doesn't do it much).

Closes #110843

[vcsjones]

change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 512 bytes in order to consume less stack.

I don't know how much of a win it is to use less stack,but in general I think it is preferable to keep a const in for the stack size. It makes it much easier to reason about "how much is going to be stack allocated" during code review and static analysis.

cc @GrabYourPitchforks Since we were just chatting about this.

[EgorBo]

change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 512 bytes in order to consume less stack.

I don't know how much of a win it is to use less stack,but in general I think it is preferable to keep a const in for the stack size. It makes it much easier to reason about "how much is going to be stack allocated" during code review and static analysis.

cc @GrabYourPitchforks Since we were just chatting about this.

I don't have a strong preference here, but in some cases it's possible that e.g. len is 4, but we still unconditionally allocate 1024 bytes for it

[EgorBo]

Related discussion: #97895

[vcsjones]

#110843 is still not fully fixed. It changes a potential stack overflow in to an access violation. This computation is done unchecked:

runtime/src/libraries/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/PerformanceData/CounterSet.cs

Lines 208 to 209 in b6038a4

	uint CounterSetInfoSize = (uint)sizeof(Interop.PerfCounter.PerfCounterSetInfoStruct)
	+ (uint)_idToCounter.Count * (uint)sizeof(Interop.PerfCounter.PerfCounterInfoStruct);

Since it is unsigned and unchecked, can overflow to a small positive value

When we try to access memory here:

runtime/src/libraries/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/PerformanceData/CounterSet.cs

Line 234 in b6038a4

CounterInfo = (Interop.PerfCounter.PerfCounterInfoStruct*)(CounterSetBuffer + CounterSetInfoUsed);

The address will be invalid.

Can be reproduced on this PR with

[Fact]
public static void BigNumberOfCounters()
{
    CounterSet counterSet = new(Guid.NewGuid(), Guid.NewGuid(), CounterSetInstanceType.Single);

    for (int i = 0; i <= 134_217_726; i++)
    {
        counterSet.AddCounter(i, CounterType.ElapsedTime);
    }

    counterSet.CreateCounterSetInstance("potato");
}

[EgorBo]

#110843 is still not fully fixed. It changes a potential stack overflow in to an access violation. This computation is done unchecked:

Thanks. Just wrapping the compution into checked context should be enough? There are many patterns where stackalloc size is computed like that, presumably, we should checked them all?

[vcsjones]

Just wrapping the computation into checked context should be enough?

Seems reasonable.

There are many patterns where stackalloc size is computed like that, presumably, we should checked them all?

I don't see a problem with it. Many will probably be unnecessary, but I don't think it introduces any overhead that is problematic.

[jkotas]

For patterns like len > CONST : stackalloc T[CONST] : new T[len] we change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 1024 bytes in order to consume less stack.

Hardcoded ad-hoc policies like this are never going be "right". The proper fix would be an API like #52065 so that one can just ask for a scratch buffer and leave the choice of strategy to the system.

[tannergooding]

I don't have a strong preference here, but in some cases it's possible that e.g. len is 4, but we still unconditionally allocate 1024 bytes for it

I think this is a case where we should be ensuring our code is correctly tuned.

If we know the vast majority of cases will be <= n bytes then that's a much better threshold to use than the default max threshold (which is generally 1024, which comes from the typical threshold used by native malloca implementations). If we have effectively two thresholds to handle (most are small, some are slightly larger), then I think that doing 2 explicit stackalloc thresholds are better (if (size <= 64) { stackalloc byte[64]; } else if (size <= 1024) { stackalloc byte[1024]; } else { RentOrAllocateNewArray; }).

Dynamic stackalloc is also "expensive", it introduces a loop that has to probe pages. All our usages will be a single page probe, which means that will be mispredicted by the CPU in the default scenario, incurring an approx 20 cycle penalty. Since stackalloc should not be getting used in known recursive functions or directly in loops, it is unlikely the predictor will ever get that trained correctly and this will be a "permanent" cost. Dynamic stackalloc also means the JIT cannot easily optimize around it, like it currently does with some small fixed allocation sizes.

I also don't think the JIT implicitly doing things to the primitive stackalloc pattern is goodness, it makes code that is likely being used in lowlevel/tuned scenarios less predictable, meaning it can't be used for it's intended scenario. I'd rather instead see us invest in actually exposing our own StackallocOrRent helper that can then do such optimizations itself, giving users the option of convenience vs control.

---

Notably dynamic length stackallocs are often considered dangerous as well. The pattern we're using here is safe (relatively speaking), but as the pattern gets copied around and used its also easy for people to misunderstand and do the wrong thing. Since we're trying to reduce unsafe code, I think being explicit and adding documentation around these areas and why certain thresholds are used/correct is a better investment; as is pushing for the helper intrinsic that generally simplifies the pattern and abstracts away the "right" sizes to use so that it can be correctly tuned, potentially participate in PGO, etc.

[EgorBo]

Reverted that part. Added checked everywhere where length is calculated near the stackalloc.
Happy to replace all places with a helper call, but seems like it won't be there soon.

[jkotas]

I think this change is introducing a bug. It looks like the code used MemoryMarshal.GetReference to avoid normalization of empty spans to null. You would have to also delete .Slice(1) above to make this change work.

[EgorBo]

Addressed

[EgorBo]

This is ready for review. Here is the code analyzer I used to find all unbound stackallocs: https://github.com/EgorBo/UnsafeCodeAnalyzer/blob/main/src/Analyzer/UnboundStackallocAnalyzer.cs (reports 263 places in Libraries and Corelib excluding tests)

[ChrML]

For patterns like len > CONST : stackalloc T[CONST] : new T[len] we change stackalloc T[CONST] to stackalloc T[len] if that CONST is >= 1024 bytes in order to consume less stack.

Hardcoded ad-hoc policies like this are never going be "right". The proper fix would be an API like #52065 so that one can just ask for a scratch buffer and leave the choice of strategy to the system.

I think if you already use "stackalloc", you should already have a good reason, you work low-level, and should have knowledge about when it's reasonable to do so. Could maybe make an analyzer that warns on stackallocing unverified dynamic lengths.

Imo it's better longterm to improve cases where normal new operations are recognized by JIT to not escape, do the length check there and automatically stackalloc them. And leave stackalloc as-is, a programmer controlled thing. Benefits existing code without changing heuristics. If this gets good enough in the future, at some point ImmutableArray.Builder with simple valuetypes populated by a loop, could occur only on the stack up to a certain number of items even putting the builder class on the stack.

I don't think simplifying ops like this is needed, except maybe being able to create scratch buffers inside methods that get allocated at callsite (such as ArrayBuilder.Create(int initialCapacity), that could return a ref struct and decide internally if put on stack or heap). Instead of passing in scratch area.

[EgorBo]

Could maybe make an analyzer that warns on stackallocing unverified dynamic lengths.

Presumably, that requires quite complex range check analysis like nullability. Our general strategy is to avoid spending efforts on low-level/unsafe things, if someone decides to go the unsafe route, they're on their own

Imo it's better longterm to improve cases where normal new operations are recognized by JIT to not escape, do the length check there and automatically stackalloc them.

Nobody argues with that, but it might take a while till we get there, the most complicated part is the Inter-procedure analysis on top of functions which might change their behavior any time (arguments suddenly start escaping) e.g. due to profilers with ReJIT attached in the middle of the flight.

I don't think simplifying ops like this is needed

I think till we get to the proper escape analysis it could be a reasonable API to simplify these patterns, I personally don't have a strong opinion on it. Another "cons" of the escape analysis is that it might be fragile and you will never know whether it kicks in or not

[ChrML]

Could maybe make an analyzer that warns on stackallocing unverified dynamic lengths.

Presumably, that requires quite complex range check analysis like nullability. Our general strategy is to avoid spending efforts on low-level/unsafe things, if someone decides to go the unsafe route, they're on their own

Imo it's better longterm to improve cases where normal new operations are recognized by JIT to not escape, do the length check there and automatically stackalloc them.

Nobody argues with that, but it might take a while till we get there, the most complicated part is the Inter-procedure analysis on top of functions which might change their behavior any time (arguments suddenly start escaping) e.g. due to profilers with ReJIT attached in the middle of the flight.

I don't think simplifying ops like this is needed

I think till we get to the proper escape analysis it could be a reasonable API to simplify these patterns, I personally don't have a strong opinion on it. Another "cons" of the escape analysis is that it might be fragile and you will never know whether it kicks in or not

Your reasoning makes very much sense, I agree.

For me personally, I think example below would increase my productivity/safety most writing low-level / high-performant code. It would help more than any language word or implicit API. Being able to write methods like this:

static MyRefStructArrayBuilder MyRefStructArrayBuilder.CreateOnStackOrHeap();
static MyRefStructArrayBuilder MyRefStructArrayBuilder.CreateOnStackOrArrayPool();

Basically moving responsability for making the stack scratch area, and where to allocate if not fitting the stack, into a common reusable place. Within the helper / builder that's used elsewhere. Rather than having this allocation at each callsite and pass it in.

No idea how it would be solved though, as you can't stack alloc inside those and return that area without some weird caller/callee contract. It'd have to be like a macro that's always inlined to the callsite or something.

Just some thoughts, maybe someone else can spin off on it, or confirm if they have the same use case as me.