Skip to content

[wasm] Handle Top-Level style entry point mangling too#47405

Merged
lewing merged 5 commits intodotnet:masterfrom
lewing:wasm-top-level-async-main
Jan 26, 2021
Merged

[wasm] Handle Top-Level style entry point mangling too#47405
lewing merged 5 commits intodotnet:masterfrom
lewing:wasm-top-level-async-main

Conversation

@lewing
Copy link
Member

@lewing lewing commented Jan 25, 2021

This adds a check for '<Main>$' in addition to 'Main' when
looking for the async entry point.

Fixes #47404

This adds a check for '<Main>$' in addition to 'Main' when
looking for the async entry point.

Fixes dotnet#47404
@lewing lewing requested a review from marek-safar as a code owner January 25, 2021 14:12
@ghost ghost added the area-VM-meta-mono label Jan 25, 2021
@ghost
Copy link

ghost commented Jan 25, 2021

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

Issue Details

This adds a check for '

$' in addition to 'Main' when
looking for the async entry point.

Fixes #47404

Author: lewing
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@lewing lewing added the arch-wasm WebAssembly architecture label Jan 25, 2021
@ghost
Copy link

ghost commented Jan 25, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This adds a check for '<Main>$' in addition to 'Main' when
looking for the async entry point.

Fixes #47404

Author: lewing
Assignees: -
Labels:

arch-wasm, area-VM-meta-mono

Milestone: -

}

// look for "Name" by trimming the first and last character of "<Name>"
async_name [name_length - 1] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

This will zero out the $, but you also need to zero out the >.

Copy link
Member Author

@lewing lewing Jan 25, 2021

Choose a reason for hiding this comment

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

Read it closer. async_name[name_length - 1] == > not $ because the length wasn't incremented

Copy link
Contributor

@CoffeeFlux CoffeeFlux Jan 25, 2021

Choose a reason for hiding this comment

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

Ah right, because you aren't null-terminating it and you end up with <Name\0$. I still think you should include the null terminator and zero out both characters.

Copy link
Member Author

@lewing lewing Jan 25, 2021

Choose a reason for hiding this comment

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

The buffer was definitely too short, switched to snprintf here (and in another place) to fix termination (and not overwrite memory). I don't see a need to null out the $ it is harmless.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Other than this and zeroing both characters, LGTM with the test. Thanks!

return args.Length;
}
} No newline at end of file
return args.Length; No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it intended to switch this to a top level program? If so, we should get rid of the indentation.

Copy link
Member Author

@lewing lewing Jan 25, 2021

Choose a reason for hiding this comment

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

no this was damage. reverting

@lewing lewing merged commit 26a21be into dotnet:master Jan 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasm WebAssembly architecture area-VM-meta-mono

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blazor Async Main and Top-Level Statements Seems to Have Issues

3 participants