Skip to content

[release/7.0][wasm] Correctly escape library names when generating symbols for .c #79175

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

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

radical
Copy link
Member

@radical radical commented Dec 2, 2022

Backport of #79007 to release/7.0

Customer impact

This allows the use of NuGets containing PInvokes that have - in their name, with WASM projects. Wasm build generates C wrapper code to call such pinvokes, and the symbol names generated in the code did not escape - in the library name.
The code now uses a different method to only have alphanumeric characters as-is, and escape/substitute any others with alphanumeric characters.

Fixes user reported issue - #78992 .

Testing

New unit tests added, and since this impacts AOT too, it got tested by all the AOT library tests.

Risk

Low. It makes the behavior consistent with symbol generation we already had in other places.

…otnet#79007)

* [wasm] Correctly escape library names when generating symbols for .c files.
Use the existing `FixupSymbolName` method for fixing library names too,
when converting to symbols.

* [wasm] *TableGenerator task: Cache the symbol name fixups
.. as it is called frequently, and for repeated strings. For a
consolewasm template build, we get 490 calls but only 140 of them are
for unique strings.

* Add tests

Fixes dotnet#78992 .

(cherry picked from commit 85a9dfc)
@radical radical added Servicing-consider Issue for next servicing release review arch-wasm WebAssembly architecture area-Build-mono labels Dec 2, 2022
@radical radical requested review from lewing, vargaz and maraf December 2, 2022 21:22
@ghost ghost assigned radical Dec 2, 2022
@ghost
Copy link

ghost commented Dec 2, 2022

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

Issue Details

Backport of #79007 to release/7.0

Customer impact

This allows the use of NuGets containing PInvokes that have - in their name, with WASM projects. Wasm build generates C wrapper code to call such pinvokes, and the symbol names generated in the code did not escape - in the library name.
The code now uses a different method to only have alphanumeric characters as-is, and escape/substitute any others with alphanumeric characters.

Fixes user reported issue - #78992 .

Testing

New unit tests added, and since this impacts AOT too, it got tested by all the AOT library tests.

Risk

Low. It makes the behavior consistent with symbol generation we already had in other places.

Author: radical
Assignees: -
Labels:

Servicing-consider, arch-wasm, area-Build-mono

Milestone: -

@rbhanda rbhanda added this to the 7.0.3 milestone Dec 6, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 6, 2022
@carlossanlop
Copy link
Contributor

Approved by Tactics for 7.0.3.
Signed-off by area owners.
CI failures: MacCatalyst known and unrelated, and a fix has been merged to the branch (#78778). The other one is a timeout cancellation.
No OOB changes needed.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 2936334 into dotnet:release/7.0 Jan 4, 2023
@radical radical deleted the pinvoke-escape-name-7.0 branch January 31, 2023 20:20
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants