Skip to content

Conversation

@lateralusX
Copy link
Member

@lateralusX lateralusX commented Mar 16, 2022

#66552 did an incorrect switch to size_t from int in a section that expects variable to go negative.

@akoeplinger
Copy link
Member

/azp run runtime-wasm

@lateralusX lateralusX force-pushed the lateralusX/fix-size-t-issue branch from 44fa240 to 913bef4 Compare March 16, 2022 17:15
dotnet#66552 did incorrect switch
to size_t from int in a sections that expects variable to go negative.
@lateralusX lateralusX force-pushed the lateralusX/fix-size-t-issue branch from 913bef4 to 8c3c2c1 Compare March 16, 2022 17:34
@lateralusX lateralusX changed the title Fix incorrect conversion from int to size_t in assembly.c. Fix incorrect conversion from int to size_t triggered by PR 66552. Mar 16, 2022
@lateralusX
Copy link
Member Author

lateralusX commented Mar 16, 2022

Found more instance with similar sign issue, added fix into this PR.

@azure-pipelines
Copy link

Comment was made before the most recent commit for PR 66721 in repo dotnet/runtime

@ghost ghost assigned lateralusX Mar 16, 2022
@ghost
Copy link

ghost commented Mar 16, 2022

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

@akoeplinger
Copy link
Member

/azp run runtime-wasm

Comment on lines +511 to 514
for (int i = ((int)nhandles - 1); i >= 0; i--) {
if (!handles_data [i])
continue;
mono_w32handle_unlock (handles_data [i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can keep gsize here like this.

Suggested change
for (int i = ((int)nhandles - 1); i >= 0; i--) {
if (!handles_data [i])
continue;
mono_w32handle_unlock (handles_data [i]);
for (gsize i = nhandles; i > 0; i--) {
if (!handles_data [i - 1])
continue;
mono_w32handle_unlock (handles_data [i - 1]);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just restored old behavior, you could argue if it should be reverse at all, also discovered that this function is not even called, so occurs to be dead code. Will remove complete instance in a different PR, but lets this one stay for now so tests can run and we can unblock the issues the other fixes in this PR should resolve.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger
Copy link
Member

The wasm Windows builds on runtime-staging are green again now, merging to unblock CI.

@akoeplinger akoeplinger merged commit a1f1ab8 into dotnet:main Mar 16, 2022
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…66721)

dotnet#66552 did incorrect switch
to size_t from int in a sections that expects variable to go negative.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants