Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: InlineArrayAttribute #61135

Closed
VSadov opened this issue Nov 3, 2021 · 68 comments
Closed

[API Proposal]: InlineArrayAttribute #61135

VSadov opened this issue Nov 3, 2021 · 68 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@VSadov
Copy link
Member

VSadov commented Nov 3, 2021

Background and motivation

There were multiple proposals and ideas in the past asking for a no-indirection primitive for inline data. Example1(inline strings), Example2(inline arrays), Example3(generalized fixed-buffers)
Our existing offering in this area – unsafe fixed-sized buffers has multiple constraints, in particular it works only with blittable value types and provides no overrun/type safety, which considerably limits its use.

In the recent hackathon we have explored a possibility of introducing a primitive for efficient, type-safe, overrun-safe indexable/sliceable inline data and it appears to be possible with relatively modest investments in the runtime.

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Struct, AllowMultiple = false)]
    public sealed class InlineArrayAttribute : Attribute
    {
        public InlineArrayAttribute (int length)
        {
            Length = length;
        }

        public int Length { get; }
    }
}

When InlineArray attribute is applied to a struct with one instance field, it is interpreted by the runtime as a directive to replicate the layout of the struct Length times. That includes replicating GC tracking information if the struct happens to contain managed pointers.

Unlike "filed0; field1; field2;..." approach, the resulting layout would be guaranteed to have the same order and packing details as elements of an array with element [0] matching the location of the single specified field.
That will allow the whole aggregate to be safely indexable/sliceable.

Length must be greater than 0.

struct must not have explicit layout.

In cases when the attribute cannot have effect, it is an error case handled in the same way as the given platform handles cases when a type layout cannot be constructed.
Generally, it would be a TypeLoadException thrown at the time of layout construction.

API Usage

// runtime replicates the layout of the struct 42 times 
[InlineArray(Length = 42)] 
struct MyArray<T> 
{ 
    private T _element0; 
    public Span<T> SliceExample() 
    { 
        return MemoryMarshal.CreateSpan(ref _element0, 42); 
    } 
} 

For more details on how this attribute could be used on the Roslyn side see Low Level Struct Improvements document.

Alternative Designs

During hackathon we have also explored an approach where the length is specified via a type parameter and arrays of concrete lengths are created by the way of generic instantiation - ValueArray<T, Size>.

Such approach moves some of the effort in creation of concrete struct arrays from the user to the runtime.
However in the current .NET IL/metadata standard, the Size parameter would need to be an integer constant encoded as a type. While such encodings are possible, finding one that everybody could comfortably accept has been an issue that we could not resolve.

Risks

InlineArrayAttribute requires runtime cooperation to function correctly. On downlevel runtimes the attribute will not have any effect, contrary to the assumptions in the code that implements slicing or indexing.

FAQ:

Why do we put the attribute on the struct and not on the field?

Allowing the attribute on individual fields introduces numerous additional scenarios and combinations that make the feature considerably more complex.

  • we would need to rationalize or somehow forbid the attribute usage on static, threadstatic, RVA fields
  • allowing replicated storage in classes would need to account for base classes that also may have instance fields, and perhaps with replicated storage as well.
  • allowing multiple replicated fields in the same struct could make the overall layout computation a fairly complex routine when field ordering, packing, ref-ness and alignment of the fields are considered. (i.e alignment would need to consider pre-replicated sizes, but packing post-replicated)

All the above issues can be solved, but at a cost to the implementation complexity while most additional scenarios appear to be less common and easily solvable by providing a wrapper struct.

@VSadov VSadov added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 3, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 3, 2021
@VSadov
Copy link
Member Author

VSadov commented Nov 3, 2021

@ghost
Copy link

ghost commented Nov 3, 2021

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

There were multiple proposals and ideas in the past asking for a no-indirection primitive for inline data. Example1(inline strings), Example2(inline arrays), Example3(generalized fixed-buffers)
Our existing offering in this area – unsafe fixed-sized buffers has multiple constraints, in particular it works only with blittable value types and provides no overrun/type safety, which considerably limits its use.

In the recent hackathon we have explored a possibility of introducing a primitive for efficient, type-safe, overrun-safe indexable/sliceable inline data and it appears to be possible with relatively modest investments in the runtime.

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Struct, AllowMultiple = false)]
    public sealed class InlineArrayAttribute : Attribute
    {
        public InlineArrayAttribute (int length)
        {
            Length = length;
        }

        public int Length { get; }
    }
}

When InlineArray attribute is applied to a struct with one instance field, it is interpreted by the runtime as a directive to replicate the layout of the struct Length times. That includes replicating GC tracking information if the struct happen to contain managed pointers.

Unlike "filed0; field1; field2;..." approach, the resulting layout would be guaranteed to have the same order and packing details as elements of an array with element [0] matching the location of the single specified field.
That will allow the whole aggregate to be safely indexable/sliceable.

Length must be greater than 0 and otherwise ignored.

API Usage

// runtime replicates the layout of the struct 42 times 
[InlineArray(Length = 42)] 
struct MyArray<T> 
{ 
    private T _element0; 
    public Span<T> SliceExample() 
    { 
        return MemoryMarshal.CreateSpan(ref _element0, 42); 
    } 
} 

For more details on how this attribute could be used on the Roslyn side see Low Level Struct Improvements document.

Alternative Designs

In hackathon we have also explored an approach where the length is specified via a type parameter and arrays of concrete lengths are created by the way of generic instantiation - ValueArray<T, Size>.

Such approach moves some of the effort in creation of concrete struct arrays from the user to the runtime.
However in the current .NET IL/metadata standard, the Size parameter would need to be an integer constant encoded as a type. While such encodings are possible, finding one that everybody could comfortably accept has been an issue that we could not resolve.

Risks

InlineArrayAttribute requires runtime cooperation to function correctly. On downlevel runtimes the attribute will not have any effect, contrary to the code that implements slicing or indexing.

As such the use of the attribute should be versioned with a mechanism such as runtime feature flags.

FAQ:

Why do we put the attribute on the struct and not on the field?

Allowing the attribute on individual fields introduces numerous additional scenarios and combinations that make the feature considerably more complex.

  • we would need to rationalize or somehow forbid the attribute usage on static, threadstatic, RVA fields
  • allowing replicated storage in classes would need to account for base classes that also may have instance fields, and perhaps with replicated storage as well.
  • allowing multiple replicated fields in the same struct could make the overall layout computation a fairly complex routine when field ordering, packing, ref-ness and alignment of the fields are considered. (i.e alignment would need to consider pre-replicated sizes, but packing post-replicated)

All the above issues can be solved, but at a cost to the implementation complexity while most additional scenarios appear to be less common and easily solvable by providing a wrapper struct.

Author: VSadov
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@jkotas jkotas added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 3, 2021
@jkotas
Copy link
Member

jkotas commented Nov 3, 2021

I think this is ready for API review.

@lambdageek
Copy link
Member

lambdageek commented Nov 3, 2021

  1. I think Length = 0 should be allowed nevermind... I think I don't like it, actualliy.

  2. What kind of validation do we want on the runtime side? E.g. throw BadImageFormatException if the length is negative non-positive, or if the struct has multiple fields.

  3. Is this ok:

     [InlineArray(Length = 10)]
     struct Tenner<T> where T : class {
        public T element0;
     }

    How about:

     [InlineArray(Length = 10)]
     struct Tenner<T> {
        public T element0;
     }

@Sergio0694
Copy link
Contributor

"During hackathon we have also explored an approach where the length is specified via a type parameter and arrays of concrete lengths are created by the way of generic instantiation - ValueArray<T, Size>."

PatrickSpongebobGIF

"Such approach moves some of the effort in creation of concrete struct arrays from the user to the runtime.
However in the current .NET IL/metadata standard, the Size parameter would need to be an integer constant encoded as a type. While such encodings are possible, finding one that everybody could comfortably accept has been an issue that we could not resolve."

SadCryingGIF

@VSadov
Copy link
Member Author

VSadov commented Nov 3, 2021

I think Length = 0 should be allowed nevermind... I think I don't like it, actualliy.

I do not think we have a choice. We cannot remove the field that is already there, thus we cannot follow the directive to have a completely empty array.

What kind of validation do we want on the runtime side? E.g. throw BadImageFormatException if the length is negative non-positive, or if the struct has multiple fields.

It must be a struct with a single instance field and not having an explicit layout
(I forgot to add the layout part to the proposal, explicit would obviously be in conflict)

I am almost 50%/50% on how we handle nonconforming situations - whether we ignore or throw an exception.
I think loader typically does not throw on misplaced attributes, but could be convinced otherwise.

Is this ok:

 [InlineArray(Length = 10)]
 struct Tenner<T> where T : class {
    public T element0;
 }

How about:

 [InlineArray(Length = 10)]
 struct Tenner<T> {
    public T element0;
 }

Yes.
I do not see anything strange about these. It is basically the target scenario similar to the example given in the proposal.
It may not be interesting to the user to make the field public, but no need to forbid that either.

@tannergooding
Copy link
Member

I do not think we have a choice. We cannot remove the field that is already there, thus we cannot follow the directive to have a completely empty array.

Why do you have to remove the field?

The intent behind 0 length fixed sized buffers in C/C++ is:

  1. Do not impact sizeof
  2. Be a marker field that you can succesfully use for indexing
  3. Much like length = 1, support the concept of "variable length inline arrays" and so must be the last field in a struct

We should be able to support and special case both 0 and 1 and have it work as expected both in sizeof calculations and in error handling or type load exceptions if they aren't the last field of a struct.

It must be a struct with a single instance field and not having an explicit layout

I think this is better stated as "must be a sequential layout struct with a single instance field", otherwise that leaves room for interpreting "auto layout" as being ok.

While such encodings are possible, finding one that everybody could comfortably accept has been an issue that we could not resolve.

I do think this is somewhat unfortunate. There are numerous patterns we could allow, particularly in the numerics space (and therefore with opportunities to improve the ecosystem for ML, Tensors, and Vector acceleration) by actually having this.

I thought the idea of using multidimensional array metadata + a special recognized constraint was a clever solution here and would allow this without having to revise IL metadata.

  • It's also then not much different from other newer constraints that we support like unmanaged or the possible new self constraint other than it being one the Runtime would enforce/recognize as well.

@lambdageek
Copy link
Member

What kind of validation do we want on the runtime side? E.g. throw BadImageFormatException if the length is negative non-positive, or if the struct has multiple fields.

It must be a struct with a single instance field and not having an explicit layout (I forgot to add the layout part to the proposal, explicit would obviously be in conflict)

I am almost 50%/50% on how we handle nonconforming situations - whether we ignore or throw an exception. I think loader typically does not throw on misplaced attributes, but could be convinced otherwise.

I think field layout errors are usually TLE or BIFE when you actually try to use the type - but I don't think we're totally precise about when the exception is raised.

I guess the alternative is that any noncomforming use of the attribute is just ignored? In that case it would be nice to have a C# analyzer flag the bad attribute usage. Unexpected layout in conjunction with pointer-ful code seems like a source of hard to diagnose bugs.

@VSadov
Copy link
Member Author

VSadov commented Nov 3, 2021

@tannergooding - I think auto layout is ok. Since we require only one instance field, only explicit could be a problem.

@jkotas
Copy link
Member

jkotas commented Nov 3, 2021

I think field layout errors are usually TLE or BIFE when you actually try to use the type

Right. Invalid uses of the attribute should produce TLE.

@tannergooding
Copy link
Member

I think auto layout is ok

I'm a bit skeptical. C# defaults to sequential structs so most users would have to go out of there way to define Auto layout in the first place. On top of that, the type layout algorithm differs in a few places for Auto layout + it prevents the usage in P/Invoke, so in almost every scenario you can think of, its undesirable to allow it IMO.

@VSadov
Copy link
Member Author

VSadov commented Nov 3, 2021

I am ok with TLE when attribute cannot work.

I assume Length == 0 is also a TLE case, right?
(a struct can't have 0 size, and we can't make [0] not work, since there is a field, so we can't effectively make Length < 1)

@tannergooding
Copy link
Member

tannergooding commented Nov 3, 2021

(a struct can't have 0 size, and we can't make [0] not work, since there is a field)

The intent behind length 0 isn't to remove the field, just to remove its from size calculations when it isn't the only field.
A struct only containing T x[0] impacts packing but not size, if you have additional fields, its no longer counted: https://godbolt.org/z/97qcx95oP

  • Clang looks to have a bug here if you have a struct that only contains T x[0] and returns 0, differing from GCC

That is, for:

#include <stddef.h>
#include <stdint.h>

struct S0 { };

size_t SizeOf_S0() { return sizeof(S0); } // 1

struct S1A { int32_t x[0]; };
struct S1B { int64_t x[0]; };

size_t SizeOf_S1A() { return sizeof(S1A); } // 1 on Win32; 4 on Win64; 0 on Clang Unix; 1 on GCC Unix
size_t SizeOf_S1B() { return sizeof(S1B); } // 1 on Win32; 8 on Win64; 0 on Clang Unix; 1 on GCC Unix

struct S2A { size_t l; int32_t x[0]; };
struct S2B { size_t l; int64_t x[0]; };

size_t SizeOf_S2A() { return sizeof(S2A); } // 4 on 32-bit; 8 on 64-bit
size_t SizeOf_S2B() { return sizeof(S2B); } // 8 on Win on Unix64; 4 on Unix32

struct S3A { size_t l; int32_t x[1]; };
struct S3B { size_t l; int64_t x[1]; };

size_t SizeOf_S3A() { return sizeof(S3A); } // 8 on 32-bit, 16 on 64-bit
size_t SizeOf_S3B() { return sizeof(S3B); } // 16 on Win and Unix64; 12 on Unix32

There should be nothing preventing the type loader from handling this, other than potential complexity, although I'd expect the complexity isn't that bad.

@jkotas
Copy link
Member

jkotas commented Nov 3, 2021

Clang looks to have a bug here if you have a struct that only contains T x[0] and returns 0

And you would like this attribute to behave like clang on Unix for Length == 0 that seems to be the most sensible behavior. Do I understand it right?

@tannergooding
Copy link
Member

And you would like this attribute to behave like clang for Length == 0 that seems to be the most sensible behavior. Do I understand it right?

Not quite. Clang's behavior here is a bug for S1A/S1B; the behavior is correct and matches GCC for S2* and S3*


The layout algorithm already special cases scenarios where the computed size is 0 and forces the managed size to 1: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/classlayoutinfo.cpp#L706-L710

We then later take into account the native size being 0:

I expect we will be tracking that it's a fixed sized buffer and the underlying length. So we should be able to do the "right thing" here by tweaking these existing spots.


That leaves us with:

  • Treat T x[n] as size being sizeof(T) * n (therefore Length = 0 gives size = 0)
  • If the total size of a struct is 0, treat it as 1
    • Win64 is special cased in that if its 0 and contains a fixed sized buffer, its also upsized to be a multiple of packing
  • If a field is to a struct where IsZeroSized() == true and it contains a zero-length inline array, don't treat it as 1 for layout purposes

This ends up:

  • matching C/C++ for Windows and Unix
  • does not remove the field
  • does not introduce a scenario where sizeof(T) could return 0

Given the typical usage scenario for Length = 0 and potential bugs otherwise, I'd expect it to be fine to place additional constraints/requirements if desired:

  • T x[0] can only be the last field of a struct
  • T x[1] is in a similar boat, its only real usage is for variable length arrays so perhaps the same constraint for it

Which would also end up with:

  • Not introducing new forms of aliasing or potential bugs where T x[0] would be at the same offset as the next field in the struct
  • Wouldn't allow T x[1] to be defined where just T x; would do, enforcing its usage to variable-lengthed array scenarios and therefore "unsafe" C# code

@VSadov
Copy link
Member Author

VSadov commented Nov 3, 2021

As I understand the suggestion is that Length == 0 would make no difference to the actual layout, it would only change how a field of such type may add or may not add to the sizeof when included in a larger struct.

I think it would be ok to allow 'Length==0` on structs with no fields. + the bizarreness with native size computation.

Are we sure this is not a UB in C ?
They'd have decades to fix/reconcile the differences by now. Is this documented ?

@tannergooding
Copy link
Member

tannergooding commented Nov 3, 2021

Are we sure this is not a UB in C ?

The Windows SDK and various GNU/Linux/MacOS native libraries use it fairly extensively, so I'm not really sure it matters if the language says "well defined" or "not" (not quite as much as Length == 1, but still frequently enough its an annoyance).

There are real type definitions in microsoft/win32metadata that should ideally be correctly encodable using Length == 0 and which should be usable in P/Invoke and interop scenarios with correct sizeof(T) reporting and behavior around address-of, indexing, etc (generally requiring unsafe indexing since they are "variable length inline arrays" in practice).

A list of the found 0-length inline arrays in the Windows SDK v10.0.20348.0 (from a basic grep, may not be exhaustive)
shared/d3dkmthk.h:285:    D3DKMT_DISPLAYMODE              pModeList[0];
shared/d3dkmthk.h:3001:    _Field_size_bytes_(Size - sizeof(_D3DKMT_OUTPUTDUPL_SNAPSHOT)) OUTPUTDUPL_CONTEXT_DEBUG_INFO OutputDuplDebugInfos[0];
shared/mstcpip.h:559:   wchar_t AllStrings[0];
shared/mstcpip.h:568:   wchar_t AllStrings[0];
shared/netiodef.h:709:    UCHAR SenderHardwareAddress[0];
shared/netiodef.h:1815:    } Block[0];
shared/netiodef.h:1847:    UINT8 Cookie[0];
shared/netiodef.h:1874:    UCHAR IpAddress[0];
shared/ntddcdrm.h:252:    CDROM_TOC_FULL_TOC_DATA_BLOCK Descriptors[0];
shared/ntddcdrm.h:276:    CDROM_TOC_FULL_TOC_DATA_BLOCK Descriptors[0];
shared/ntddcdrm.h:335:    CDROM_TOC_ATIP_DATA_BLOCK Descriptors[0];
shared/ntddcdrm.h:375:    CDROM_TOC_CD_TEXT_DATA_BLOCK Descriptors[0];
shared/ntddcdrm.h:894:    UCHAR                        Data[0];
shared/ntddcdvd.h:131:    UCHAR KeyData[0];
shared/ntddcdvd.h:240:    UCHAR Data[0];
shared/ntddcdvd.h:287:    UCHAR BCAInformation[0];
shared/ntddcdvd.h:368:    UCHAR RMDBytes[0];
shared/ntddcdvd.h:544:    DVD_DISC_CONTROL_BLOCK_LIST_DCB Dcbs[0];
shared/ntddcdvd.h:580:    // DVD_LIST_OF_RECOGNIZED_FORMAT_LAYERS_TYPE_CODE TypeCodes[0];
shared/ntddmmc.h:57:    UCHAR Data[0];            // extra data, typically FEATURE_HEADER
shared/ntddmmc.h:197:    FEATURE_DATA_PROFILE_LIST_EX Profiles[0];
shared/ntddmmc.h:304:    UCHAR LinkSize[0];
shared/ntddmmc.h:487:    UCHAR LinkSizes[0];
shared/ntddmmc.h:711:    UCHAR SerialNumber[0];
shared/ntddmmc.h:729:    FEATURE_DATA_DISC_CONTROL_BLOCKS_EX Data[0];
shared/ntddmmc.h:775:    UCHAR Data[0];
shared/ntddmmc.h:783:    UCHAR VendorSpecificData[0];
shared/ntddscsi.h:1010:        NVCACHE_PRIORITY_LEVEL_DESCRIPTOR   Priority[0];
shared/ntddscsi.h:1207:    STORAGE_FIRMWARE_SLOT_INFO Slot[0];
shared/ntddscsi.h:1227:    STORAGE_FIRMWARE_SLOT_INFO_V2 Slot[0];
shared/ntddscsi.h:1248:    UCHAR       ImageBuffer[0];     // firmware image file. 
shared/ntddscsi.h:1264:    UCHAR       ImageBuffer[0];     // firmware image file. 
shared/ntddstor.h:5459:    UCHAR SerialNumber[0];
shared/ntddstor.h:5655:            UCHAR ParameterList[0];
shared/nvme.h:332:    ULONG                           Doorbells[0];       // Start of the first Doorbell register. (Admin SQ Tail Doorbell)
shared/scsi.h:1633:    UCHAR ClassEventData[0];
shared/scsi.h:1797:    UCHAR SupportedSecurityProtocol[0];
shared/scsi.h:1825:    UCHAR Data[0];
shared/scsi.h:1852:    UCHAR Data[0];
shared/scsi.h:1915:    FORMAT_DESCRIPTOR Descriptors[0];
shared/scsi.h:1938:    FORMATTED_CAPACITY_DESCRIPTOR Descriptors[0];
shared/scsi.h:2072:    OPC_TABLE_ENTRY OPCTable[0];
shared/scsi.h:2874:    UCHAR SerialNumber[0];
shared/scsi.h:2887:    UCHAR SerialNumber[0];
shared/scsi.h:2935:    UCHAR Identifier[0];
shared/scsi.h:2954:    // VPD_IDENTIFICATION_DESCRIPTOR Descriptors[0];
shared/scsi.h:2955:    UCHAR Descriptors[0];
shared/scsi.h:3108:        UCHAR Descriptors[0];
shared/scsi.h:3176:    UCHAR ProvisioningGroupDescr[0];
shared/scsi.h:3215:    UCHAR SupportedPageList[0];
shared/scsi.h:3306:        UCHAR AsByte[0];                        // Bytes 4-N
shared/scsi.h:3394:    LOG_PARAMETER Parameters[0];
shared/scsi.h:3437:    LOG_PARAMETER_HEADER Parameters[0];
shared/scsi.h:3520:    PRI_RESERVATION_DESCRIPTOR Reservations[0];
shared/scsi.h:4398:    LBA_STATUS_DESCRIPTOR Descriptors[0];
shared/scsi.h:5290:    UNMAP_BLOCK_DESCRIPTOR Descriptors[0];
shared/srb.h:445:    OUT BOOLEAN SupportedTypeList[0];
shared/usbioctl.h:681:    USB_PIPE_INFO PipeList[0];
shared/usbioctl.h:702:USB_PIPE_INFO PipeList[0];
shared/usbioctl.h:928:    USB_PIPE_INFO PipeList[0];/* OUTPUT */
shared/usbioctl.h:978:    UCHAR Data[0];
shared/usbioctl.h:1028:    USB_PIPE_INFO PipeList[0];/* OUTPUT */
um/ClusApi.h:5762:    FILESHARE_CHANGE    ChangeEntry[0];
um/MSWSock.h:479:    WSAPOLLFD fdArray[0];
um/Routprot.h:615:    _Field_size_(AddressCount) IP_LOCAL_BINDING    Address[0];
um/Routprot.h:629:    _Field_size_(AddressCount) IPV6_LOCAL_BINDING      Address[0];
um/mfapi.h:983:BYTE SliceHeaderBytes[0];               // slice header data, the last byte might contain some bits not used, leave them random
um/mfapi.h:991:SliceHeader rgstSliceheader[0];         // cNumHeaders slice header data
um/minidumpapiset.h:101:    WCHAR   Buffer [0];     // Variable size buffer
um/minidumpapiset.h:311:    MINIDUMP_THREAD Threads [0];
um/minidumpapiset.h:331:    MINIDUMP_THREAD_EX Threads [0];
um/minidumpapiset.h:397:    MINIDUMP_MEMORY_DESCRIPTOR MemoryRanges [0];
um/minidumpapiset.h:403:    MINIDUMP_MEMORY_DESCRIPTOR64 MemoryRanges [0];
um/minidumpapiset.h:726:    MINIDUMP_THREAD_NAME ThreadNames[0]; // Variable size buffer
um/winioctl.h:5614:    BYTE  SerialNumber[0];
um/winioctl.h:5810:            BYTE  ParameterList[0];
um/winnt.h:19737://  IMAGE_DYNAMIC_RELOCATION DynamicRelocations[0];
um/winnt.h:19749://  IMAGE_BASE_RELOCATION BaseRelocations[0];
um/winnt.h:19755://  IMAGE_BASE_RELOCATION BaseRelocations[0];

@jaredpar
Copy link
Member

jaredpar commented Nov 4, 2021

One part that worries me is that the pattern here of slicing the inline array is hard to write correctly. It's very easy though to write as the example in the issue does and that creates a convenient yet large type safety hole.

Span<int> Oops() {
  MyArray<int> array = default; 
  return array.SliceExample();
}

This will compile just fine and it's very unsafe. In general I think if our API reviews need MemoryMarshal.Unsafe in the prime examples we should pause and think about whether or not we're pushing people towards more unsafe patterns.

As we look to approve this in API review I think we should also be looking to promoting the correct and safe usage patterns around it. It's tricky though because the nature of this API essentially goes against the protections in the span safety rules. We may need to consider pairing this with a language change.

@jkotas
Copy link
Member

jkotas commented Nov 4, 2021

My assumption was that this is enabling feature for https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md#safe-fixed-size-buffers and the buffer will by typically accessed by auto-generated accessor that returns ReadOnlySpan<T>, except for the Length = 0 cases discussed above where the memory is manually managed and the compiler cannot infer the actual length of the buffer.

@jaredpar
Copy link
Member

jaredpar commented Nov 4, 2021

I agree the primary use would be to enable that feature. I'm also trying to think of the general use case here. Essentially it's an API we're offering and what will be the implications of direct usage? Other features in that proposal, particularly offering the ability to invert our lifetime defaults, will make usage of this safe. I worry a bit though about taking this without the other though because the inclination is likely to be to go for the unsafe uses.

@jkotas
Copy link
Member

jkotas commented Nov 4, 2021

FWIW, using Spans with fixed buffers has the lifetime type safety hole today. Example from this repo:

internal ReadOnlySpan<char> cFileName
{
get { fixed (char* c = _cFileName) return new ReadOnlySpan<char>(c, MAX_PATH); }
}

I worry a bit though about taking this without the other though because the inclination is likely to be to go for the unsafe uses.

I agree that we should commit to take both or neither for the release.

@tannergooding
Copy link
Member

tannergooding commented Jan 25, 2023

Note: Asking the below because of my own interest in this, but also because I've seen a number of community members and other people interested in this area asking questions (such as on the C# Community Discord) where the answers don't seem to be clear/concise.


Could I get a couple questions answered as to how a few things are impacted.

  1. Is [InlineArray] going to be usable as part of method parameters. That is, could you declare for example Foo(float x[4]) (just using the C/C++ syntax here for simplicity).
  2. How are assignments going to work between them. Some [InlineArray] declared in struct S wouldn't be assignment compatible (by default) with some [InlineArray] declared in struct S2 since they would be different types
  3. How are we expected to utilize inline arrays in areas outside of fields, such as type parameters, function pointers, etc.

Another question is specifically around the proposed but "we couldn't agree on an approach" for some InlineArray<T, #> approach and whether we can get a concrete summary of what the issues were and where the disagreements existed.

The approach of allowing numbers as part of a template is a powerful feature and one that is much more broadly applicable/useful to the .NET ecosystem, especially in the numerics and perf oriented domains. Trying to shuffle in the InlineArray feature and not considering this other aspect seems like a major downside and one that might come back to bite us later.

That is, why aren't we using a multidimensional array or even "Size" parameter: #60519 (comment) -- That is, IL already allows int[5] or int[5, 6]`, etc. We could expose this completely unambiguously even for multidimensional inline arrays without having to specify large numbers of ranks or other unnecessary metadata, so why aren't we?

@DaZombieKiller
Copy link
Contributor

The approach of allowing numbers as part of a template is a powerful feature and one that is much more broadly applicable/useful to the .NET ecosystem, especially in the numerics and perf oriented domains.

Regarding this, several approaches for how to encode constant values in generic parameters were discussed in the C# Discord. To support ValueArray<T, int Size>, MyClass<T, float F, int N>, MyClass<T, string N>, etc. F/N could be a metadata token for a constant field encoded with an array length or rank. The constant field would contain the actual value to use (and would be emitted by the compiler).

For primitives that fit in 32-bits or less, it might be worth foregoing using a metadata token and using the array length/rank value directly. A runtime-recognized MetadataTokenAttribute could then be applied to the generic parameter when it is a larger type such as long, string or double.

This still requires agreeing on which method is appropriate for storing these values in IL, though. It would be very interesting to hear more about the discussions that occurred on this subject for ValueArray<T, int Size>.

@jaredpar
Copy link
Member

The approach of allowing numbers as part of a template is a powerful feature and one that is much more broadly applicable/useful to the .NET ecosystem, especially in the numerics and perf oriented domains

I agree it's much more powerful, it's also much more expensive. That is a significant revamping of the C# and IL type systems that is simply not necessary to solve the set of challenges that we're looking at right now. The current solution allows us to solve a number of outstanding problems such as safe fixed fields and params ReadOnlySpan<T>. That is a significant benefit to the ecosystem.

The proposed solution does not preclude expanding to numeric template arguments in the future.

Practically any feature we discuss can be done in a more expanded and powerful form so long as we're willing to up the complexity and resource investment.

How are assignments going to work between them. Some [InlineArray] declared in struct S wouldn't be assignment compatible (by default) with some [InlineArray] declared in struct S2 since they would be different types

This is part of the details we still need to work out. The current proposal suggests that the fields are not directly accessible but rather accessible through Span<T> / ReadOnlySpan<T> instances. That removes the issue of direct assignment and instead puts into known territory regarding copying spans.

How are we expected to utilize inline arrays in areas outside of fields, such as type parameters, function pointers, etc.

By using the types which are annotated with [InlineArray]

Note: I'm somewhat ambivalent to the implementation here but this solution has a pretty understood mapping to the key C# scenarios we're trying to solve.

@terrajobst
Copy link
Member

I assume this means the proposal is still approved as-is? Or is more discussion needed and we should reset it to api-needs-work?

@tannergooding
Copy link
Member

tannergooding commented Jan 26, 2023

I think its probably "fine" as-is, but I think there needs to be a FAQ explaining the "why" this route is being done (ideally as part of the top post so it's easily findable). This may particularly be the case given the additional number of hearts/thumbs up on my comment above.

Ideally such a FAQ would also clearly cover things like how this will be exposed so that it doesn't prevent migration to some future ValueArray<T, #> or similar.

@Sergio0694
Copy link
Contributor

"I agree it's much more powerful, it's also much more expensive."

I just have a question on this point specifically. I understand that adding generic constants would be more work, but given that that would completely supersede the proposed [InlinedArray] "workaround", which is less work but still not free, this would mean that the current path requires doing more work overall just to get a subset of the features shipped sooner, right? As in, I assume this has been considered and doing a fair amount of extra overall work (as well as accepting having to deal with more complexity that will just have to forever remain in the runtime to keep supporting this) was still worth it? 🥲

@jkotas
Copy link
Member

jkotas commented Jan 26, 2023

I understand that adding generic constants would be more work

It would be close to two orders of magnitude more work all up. The throw-away work for InlineArray is noise compared to that.

@hez2010
Copy link
Contributor

hez2010 commented Feb 28, 2023

Just putting a note that by using InlineArray approach you are accumulating tech debt that will need to be paid for eventually.

@tannergooding
Copy link
Member

@VSadov, there was a small discussion on Length == 0 above (#61135 (comment)).

Could you remind me on what was the outcome of that? I gave some examples on how it is spec'd to work for the Windows vs Linux ABI and listed some examples of Win32 APIs that utilize it. I don't, however, see any follow up on whether we decided to explicitly say that we're going to keep the restriction that Length > 0 and therefore such types are "out of scope" or if the spec needs to be updated to account for them.

@VSadov
Copy link
Member Author

VSadov commented Mar 16, 2023

There was no change for that. The purpose of the attribute is to replicate the storage of the existing field and it will require Length > 0.

T[0] may still be a valid type from C# point of view and language may choose whichever of the behaviors described in this thread. - I.E allowing such arrays only as the last field, etc.
There is no need to map that to InlineArray though.

@tannergooding
Copy link
Member

There is no need to map that to InlineArray though.

It won't work "as expected" from an ABI perspective if the runtime doesn't special case it.

I think that's fine, just wanted to confirm there was no change and that we're explicitly considering it out of scope.

@hez2010
Copy link
Contributor

hez2010 commented Jul 20, 2023

Is [InlineArray] going to be usable as part of method parameters. That is, could you declare for example Foo(float x[4]) (just using the C/C++ syntax here for simplicity).

Allowing InlineArray to be exposed using T[N] such as int[4] in public APIs is extremely dangerous. Two InlineArrays with the same size, element type and layout from different assemblies will NOT be compatible because they are defined separately, which means:

In assembly A:

class Foo1
{
    public static void Use(int buffer[4]) { }
}

and in assembly B you want to call A.Use:

class Foo2
{
    public static void Test(int buffer[4])
    {
        Foo1.Use(buffer); // error: because the buffer[4] here is lowered to a compiler-generated
                          // InlineArray type defined in the assembly B, while the buffer[4] in Foo1.Use is
                          // lowered to a compiler-generated InlineArray type defined in the assembly A,
                          // so, they are not the same type and are not compatible.
    }
}

This is leading to critical problem to the ABI and type system and is completely unacceptable.

Definitely, you can predefine some limited types in the BCL, such as FixedBuffer1<T>, FixedBuffer2<T>, ..., but this is significantly different from the Action/Func types. In a method call, the number of parameters is usually less or equal than 16 so you can assume there won't be too many use cases where the number of parameters is greater than 16. However, as for buffer, we may simply hit the limit of 16, and it is pretty common to use a fixed buffer which contains more than 100 elements. A common use case is to use a fixed buffer to receive data from socket which may need FixedBuffer512<byte>. You may want to argue me with the statement "why not use stackalloc here?", but stackalloc simply doesn't work in async code. I believe you don't want to define hundreds distinct types for this in the BCL.

If we indent to allow to expose fixed buffer types in the public API, we must consider a more general solution, for example. supporting integer values in the type parameters so we can make a public struct FixedBuffer<T, int N> common type in the BCL, and then we can avoid the above problem:

// in assembly A
class Foo1
{
    public static void Use(int buffer[4]) { } // lowered to FixedBuffer<int, 4>
}

// in assembly B
class Foo2
{
    public static void Test(int buffer[4]) // lowered to FixedBuffer<int, 4>
    {
        Foo1.Use(buffer); // ok
    }
}

@KalleOlaviNiemitalo
Copy link

If a parameter of a method has an inline buffer type, and the corresponding argument has a different inline buffer type, but the element type and the size are the same, then could the compiler solve the type mismatch by emitting a call to a method like Unsafe.As<TFrom, TTo>(ref TFrom), except modified to verify binary compatibility at JIT compilation time?

@hez2010
Copy link
Contributor

hez2010 commented Jul 20, 2023

could the compiler solve the type mismatch by emitting a call to a method like Unsafe.As<TFrom, TTo>(ref TFrom),

I believe you won't want this workaround. For example, how to handle reflection? Should all existing dependency injection frameworks introduce exceptions for InlineArray types? And not only the runtime, but all existing and new code which need to resolve types at runtime will need to special casing the InlineArray types. Or you want to special case it in the runtime so that we can pass a value of Foo type to the code which receives a Bar? It's ridiculous and will make the type system unreliable.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jul 20, 2023

Should all existing dependency injection frameworks introduce exceptions for InlineArray types?

If a method of an interface takes an inline array parameter, and DI injects an implementation of this interface, then the implementing method must already use the same inline array type; otherwise the implementing type could not have been compiled.

If a constructor takes an inline array parameter and DI is supposed to provide an inline array instance as an argument, then a conversion could be needed; but that seems an unlikely scenario, as the element type and size of the array would not sufficiently identify the purpose of the instance.

If a constructor takes a Func<SomeCustomType, byte[32]> parameter (where SomeCustomType is distinctive enough) and DI is supposed to provide a delegate instance as an argument, then a conversion could be needed; and to implement that conversion, DI might have to wrap the delegate behind another delegate. But the developer can replace the delegate type with an interface and then it's the same as the first case.

So no, I don't think DI would need to support the conversion. It would be a bit like casting int[] to uint[], which is supported by the runtime but AFAIK not by DI frameworks.

Or you want to special case it in the runtime

No, I'm only thinking about special-casing it in the compiler. Although I don't know if such a typecast hack would still cause some problem in the runtime, even when the element type and the size match.

@hez2010
Copy link
Contributor

hez2010 commented Jul 20, 2023

So no, I don't think DI would need to support the conversion. It would be a bit like casting int[] to uint[], which is supported by the runtime but AFAIK not by DI frameworks.

DI is only a use case of reflection. This behavior will affect all reflection usage.

My point is simple: if we can introduce integer const support in generic parameter in IL directly, all those problems are automatically gone and no workaround is needed here.

Rust also didn't have const generics a while ago and it used macros to generate types just like what C# is doing now, this approach has been proved by Rust that it's limited and has issues around ABI.
But now Rust has brought the support for const generics to the language and the whole ecosystem benefits from this feature.
I don't think dotnet must replicate the history of Rust, we already have the experience thanks to Rust so why not skip such tricks and bring the native support for const generics directly? It does cost more time to implement but I don't see any urgency of rushing the InlineArray.

@KalleOlaviNiemitalo
Copy link

Did Rust get binary compatibility or source compatibility between the macro-based system and the generics-based system, or do libraries have to choose which system they support in their APIs?

Thinking about how, if .NET libraries first deploy InlineArrayAttribute-based APIs and a generics-based alternative becomes available later, the libraries might be unable to migrate because of compatibility requirements.

@hez2010
Copy link
Contributor

hez2010 commented Jul 20, 2023

If .NET libraries first deploy InlineArrayAttribute-based APIs and a generics-based alternative becomes available later, the libraries might be unable to migrate because of compatibility requirements

We currently don't have the proposed fixed buffer language syntax support so it's okay that the InlineArray is for internal use. But if we expose InlineArray to the public and ship the syntax in the language, it will be an uncorrectable mistake and we end up with living with the issue forever.

@tannergooding
Copy link
Member

Closing as this was implemented and is in .NET 8

@ghost ghost locked as resolved and limited conversation to collaborators Nov 20, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: Future, 8.0.x Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests