Fix formatting string for assembly MVID mismatch #50474
Conversation
src/coreclr/vm/nativeimage.cpp
Outdated
| @@ -285,7 +285,7 @@ void NativeImage::CheckAssemblyMvid(Assembly *assembly) const | |||
| StringFromGUID2(*componentMvid, componentMvidText, MVID_TEXT_LENGTH); | |||
|
|
|||
| SString message; | |||
| message.Printf(W("MVID mismatch between loaded assembly '%s' (MVID = %s) and an assembly with the same simple name embedded in the native image '%s' (MVID = %s)"), | |||
| message.Printf(W("MVID mismatch between loaded assembly '%S' (MVID = %s) and an assembly with the same simple name embedded in the native image '%s' (MVID = %s)"), | |||
There was a problem hiding this comment.
It seems we could also change the native image formatting to %S and replace the SString(SString::Utf8, GetFileName()).GetUnicode() by just GetFileName(). Or maybe the other way - keep the %s where you've changed it to %S and change the related argument to SString(SString::Utf8, assembly->GetSimpleName()).GetUnicode().
The reason I am suggesting the other way is that I am not sure if message.Printf can handle UTF-8 strings (maybe it would consider the 8 bit strings to be encoded using something like Windows 1250). Can you please test it with some assembly name that would have non-ascii characters in the name to make sure it handles them properly?
The simpler of way without re-encoding to UTF16 would be preferred if it works.
There was a problem hiding this comment.
Thanks Jan for pointing that out, I have verified locally that Printf indeed prints complete nonsense when presented with a UTF-8 string. I have modified the code to use Unicode per your suggestion. Based on our offline discussion I also had to update the CrstTypes.def file to declare the fact that the AssemblyLoader lock may now also need to take DebuggerMutex in the case when the MVID validation fails and the application fails fast.
|
The field offset verification fix is still hitting issues on ARM and will likely take a few more hours to land; for this reason I'm retesting this PR in the hope that after JanK's integration of the NullRef fix it starts passing and can be merged in immediately. |
During development of the MVID check change I at some point ended up with mismatched printf specifiers and arguments - the string is Unicode so that we need %S to print the 8-bit string returned from GetSimpleName. Thanks Tomas
trylek
force-pushed
the
MvidMismatchErrorMessageFix
branch
from
8dd14d2
to
64b9741
Compare
|
I believe I have addressed all PR feedback and the latest run hasn't (yet) uncovered any new issues, the single-file leg seems affected by a network failure. Can you please take another look when you have a chance and let me know if you think the change is good to go in and start untangling our various CG2 pending issues? |
ghost
locked as resolved and limited conversation to collaborators
During development of the MVID check change I at some point ended
up with mismatched printf specifiers and arguments - the string
is Unicode so that we need %S to print the 8-bit string
returned from GetSimpleName.
Thanks
Tomas
/cc @dotnet/crossgen-contrib