[mono][wasm] Fix function signature mismatch in m2n invoke #101106
Conversation
ghost
added
the
|
Tagging subscribers to 'arch-wasm': @lewing |
lewing
requested review from
maraf,
akoeplinger,
ilonatommy,
lewing and
pavelsavara
as code owners
|
Failures are not related. |
| // Append the hexadecimal representation of b between underscores | ||
| sprintf(&fixedName[sb_index], "_%X_", b); | ||
| sb_index += 4; // Move the index after the appended hexadecimal characters | ||
| } |
There was a problem hiding this comment.
we need to add a check that sb_index is < 252 (to account for the possible 4 hex bytes + 1 null terminate) in the loop so we don't write past the fixedName boundary.
There was a problem hiding this comment.
and ideally use snprintf to make it clear the code is doing this
|
|
||
| // Null-terminate the fixedName string | ||
| fixedName[sb_index] = '\0'; | ||
| return fixedName; |
There was a problem hiding this comment.
all of the call sites need to free this or we're leaking the memory.
There was a problem hiding this comment.
Thanks , will create a PR for these changes.
| #ifdef TARGET_WASM | ||
| acfg->static_linking_symbol = g_strdup (mono_fixup_symbol_name(symbol)); | ||
| #else | ||
| /* Get rid of characters which cannot occur in symbols */ |
There was a problem hiding this comment.
I wonder why this needs to be wasm-specific, shouldn't we do the same logic everywhere?
There was a problem hiding this comment.
It broke iOS/tvOS tests that's why I applied the logic only for wasm
There was a problem hiding this comment.
hmm that sounds concerning, I don't see why it should break iOS/tvOS
There was a problem hiding this comment.
This is what error looked like (from https://dev.azure.com/dnceng-public/public/_build/results?buildId=652242&view=logs&j=88e1dda0-08c6-5d85-6ee4-8e42aeff31b6&t=b4bb5080-7a46-5114-c100-ade75b904ada)
My guess is that for iOS/tvOS in other places it still expects the previous logic. And I'm not sure that for other platforms there is a need to apply the change.
There was a problem hiding this comment.
then we need to find these places, the logic should be consistent
| } | ||
| else | ||
| { | ||
| sb.Append($"_{b:X}_"); |
There was a problem hiding this comment.
I'm not sure of the exact constraints for this name mapping algorithm but could we just replace all chars outside of a-z/0-9 with underscore? that would allow us to not allocate in the native side
There was a problem hiding this comment.
This is needed when mapping two assemblies to the same name.
For example having an app project that references a library project named in a way that maps to the same name, but is different before fixup.
There was a problem hiding this comment.
Ah so e.g. HelloÖProject.DoSomething() and HelloÜProject.DoSomething() would both map to Hello_Project_DoSomething, makes sense.
There was a problem hiding this comment.
not sure whether it'd be strictly better but we could also collect the special chars and append them at the end or return them separately. that'd have the benefit of not needing to allocate on the native side when the string doesn't have special chars
@lambdageek for thoughts
There was a problem hiding this comment.
I'm not sure how adding the characters in a suffix would make a difference.
On the native side I guess it would be nice to have a function that precomputes the required space for the entire mangled name and just does a single allocation. (ie: get rid of the g_strdup_printf with the prefix and the name together with the mangling loop - and just make a helper that that does a single allocation for the final mangled name)
…1106) Fix signature mismatch
…1106) Fix signature mismatch
While working on this PR found #101434 and #101476.
Once it will be fixed test cases will be updated accordingly.
Fixes #99514