-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23865
Conversation
…#23817) * Fix #23680 * PR Feedback * More tests * More tests
@dotnet/dnceng, do you know what's going on with these build failures? |
@dotnet-bot test this please |
@dagood Do you mind taking a look into what is going on here? |
I'm able to repro on my dev machine by checking out this branch (macOS 10.12.6).
I'm not familiar with how to debug a SIGABRT, but it doesn't appear infra-related. To get a bit more info, I tried a build on This is the command I'm using: |
column += copyCount; | ||
} | ||
} | ||
int CopyToCore(T[] sourceBuffer, int sourceIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this to private int CopyToCore(T[], int, T[] array, int arrayIndex, int count)
in the class lets the build command pass on my dev box. Seems like a product bug that compiling this crashes on OSX. (Edit: Now that other CI runs are coming in, it seems that this repros everywhere.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using different compiler versions in release/2.0 vs master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at Tools/tool-runtime/project.csproj
Microsoft.Net.Compilers.netcore
:
- release/2.0.0:
2.0.0-rc
- master:
2.0.0-rc4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dagood, do you know why we're still using old(er) preview bits? At the very least I would have thought we'd be using a shipped compiler, or if not, it'd be because we're using something cutting edge.
@OmarTawfik, in the name of moving this forward, can you avoid using the local function?
@agocke, you implemented local functions, right? Does this crash sound familiar at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dagood, do you know why we're still using old(er) preview bits?
There's an upgrade going on in dotnet/buildtools#1661, but I don't have any broader context on why certain versions are being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub I'll avoid the local function for now.
@stephentoub @dagood both NT builds are now failing because of this test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This 2.0 branch so this test expected to fail when running on Windows versions (I think rs2 or up). we fixed the test on the master branch. so you can ignore this failure |
approved |
Porting #23817 to release 2.0 branch.
Fixes #23680