From be670ac7633d451e21667cd5cf0716d94ff3baf8 Mon Sep 17 00:00:00 2001 From: tijoytk Date: Mon, 9 May 2016 16:23:52 -0700 Subject: [PATCH] Taking care of review comments. Let out some of the review comments since they are optimizations. Commit migrated from https://github.com/dotnet/coreclr/commit/515a28b8ab33675011af9923bb5b6dfedf5e3935 --- src/coreclr/src/mscorlib/src/System/StubHelpers.cs | 27 ++++++++++----------- src/coreclr/src/vm/fieldmarshaler.cpp | 22 ++++++++++------- src/coreclr/src/vm/marshalnative.h | 2 ++ .../src/Interop/StringMarshalling/UTF8/UTF8Test.cs | Bin 17888 -> 9308 bytes .../StringMarshalling/UTF8/UTF8TestNative.cpp | 17 +++++-------- 5 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/coreclr/src/mscorlib/src/System/StubHelpers.cs b/src/coreclr/src/mscorlib/src/System/StubHelpers.cs index 3a5ece6..8d47482 100644 --- a/src/coreclr/src/mscorlib/src/System/StubHelpers.cs +++ b/src/coreclr/src/mscorlib/src/System/StubHelpers.cs @@ -128,7 +128,7 @@ namespace System.StubHelpers { [ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)] internal static class UTF8Marshaler { - const int UTF8_CHAR_SIZE = 3; + const int MAX_UTF8_CHAR_SIZE = 3; static internal unsafe IntPtr ConvertToNative(int flags, string strManaged, IntPtr pNativeBuffer) { if (null == strManaged) @@ -142,21 +142,18 @@ namespace System.StubHelpers { // If we are marshaling into a stack buffer allocated by the ILStub // we will use a "1-pass" mode where we convert the string directly into the unmanaged buffer. - // else we will allocate the precise native heap memory. + // else we will allocate the precise native heap memory. if (pbNativeBuffer != null) { // this is the number of bytes allocated by the ILStub. - nb = (strManaged.Length + 1) * UTF8_CHAR_SIZE; - - // +1 for the '\0' that we add - nb += 1; + nb = (strManaged.Length + 1) * MAX_UTF8_CHAR_SIZE; // nb is the actual number of bytes written by Encoding.GetBytes. // use nb to de-limit the string since we are allocating more than // required on stack nb = strManaged.GetBytesFromEncoding(pbNativeBuffer, nb, Encoding.UTF8); } - // required bytes > 260 , allocate required bytes on heap + // required bytes > 260 , allocate required bytes on heap else { nb = Encoding.UTF8.GetByteCount(strManaged); @@ -164,7 +161,7 @@ namespace System.StubHelpers { pbNativeBuffer = (byte*)Marshal.AllocCoTaskMem(nb + 1); strManaged.GetBytesFromEncoding(pbNativeBuffer, nb, Encoding.UTF8); } - pbNativeBuffer[nb] = 0x0; + pbNativeBuffer[nb] = 0x0; return (IntPtr)pbNativeBuffer; } @@ -217,12 +214,14 @@ namespace System.StubHelpers { int nbBytes = StubHelpers.strlen((sbyte*)pNative); int numChar = Encoding.UTF8.GetCharCount((byte*)pNative, nbBytes); - // Encoding.UTF8.GetChars throw an argument exception - // if pBuffer is null - if (numChar == 0) - return; - - char[] cCharBuffer = new char[numChar]; + // +1 GetCharCount return 0 if the pNative points to a + // an empty buffer.We still need to allocate an empty + // buffer with a '\0' to distingiush it from null. + // Note that pinning on (char *pinned = new char[0]) + // return null and Encoding.UTF8.GetChars do not like + // null argument. + char[] cCharBuffer = new char[numChar + 1]; + cCharBuffer[numChar] = '\0'; fixed (char* pBuffer = cCharBuffer) { numChar = Encoding.UTF8.GetChars((byte*)pNative, nbBytes, pBuffer, numChar); diff --git a/src/coreclr/src/vm/fieldmarshaler.cpp b/src/coreclr/src/vm/fieldmarshaler.cpp index 39465c2..23cbe18 100644 --- a/src/coreclr/src/vm/fieldmarshaler.cpp +++ b/src/coreclr/src/vm/fieldmarshaler.cpp @@ -3190,12 +3190,14 @@ VOID FieldMarshaler_StringUtf8::UpdateNativeImpl(OBJECTREF* pCLRValue, LPVOID pN COMPlusThrow(kMarshalDirectiveException, IDS_EE_STRING_TOOLONG); // Characters would be # of characters + 1 in case left over high surrogate is ? - // Max 3 bytes per char for basic multi-lingual plane. - nc = (nc + 1) * 3; + // Max 3 bytes per char for basic multi-lingual plane. + nc = (nc + 1) * MAX_UTF8_CHAR_SIZE; // +1 for '\0' LPUTF8 lpBuffer = (LPUTF8)CoTaskMemAlloc(nc + 1); if (!lpBuffer) + { COMPlusThrowOM(); + } // UTF8Marshaler.ConvertToNative MethodDescCallSite convertToNative(METHOD__CUTF8MARSHALER__CONVERT_TO_NATIVE); @@ -3221,18 +3223,20 @@ VOID FieldMarshaler_StringUtf8::UpdateCLRImpl(const VOID *pNativeValue, OBJECTRE CONTRACTL { THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - INJECT_FAULT(COMPlusThrowOM()); - PRECONDITION(CheckPointer(pNativeValue)); - PRECONDITION(CheckPointer(ppProtectedCLRValue)); + GC_TRIGGERS; + MODE_COOPERATIVE; + INJECT_FAULT(COMPlusThrowOM()); + PRECONDITION(CheckPointer(pNativeValue)); + PRECONDITION(CheckPointer(ppProtectedCLRValue)); } CONTRACTL_END; STRINGREF pString = NULL; LPCUTF8 sz = (LPCUTF8)MAYBE_UNALIGNED_READ(pNativeValue, _PTR); if (!sz) + { pString = NULL; + } else { MethodDescCallSite convertToManaged(METHOD__CUTF8MARSHALER__CONVERT_TO_MANAGED); @@ -3260,10 +3264,10 @@ VOID FieldMarshaler_StringUtf8::DestroyNativeImpl(LPVOID pNativeValue) const } CONTRACTL_END; - LPSTR lpBuffer = (LPSTR)MAYBE_UNALIGNED_READ(pNativeValue, _PTR); + LPCUTF8 lpBuffer = (LPCUTF8)MAYBE_UNALIGNED_READ(pNativeValue, _PTR); MAYBE_UNALIGNED_WRITE(pNativeValue, _PTR, NULL); if (lpBuffer) - CoTaskMemFree(lpBuffer); + CoTaskMemFree((LPVOID)lpBuffer); } //======================================================================= diff --git a/src/coreclr/src/vm/marshalnative.h b/src/coreclr/src/vm/marshalnative.h index a6c809d..4f6ac85 100644 --- a/src/coreclr/src/vm/marshalnative.h +++ b/src/coreclr/src/vm/marshalnative.h @@ -15,6 +15,8 @@ #include "fcall.h" +#define MAX_UTF8_CHAR_SIZE 3 + //!!! Must be kept in sync with ArrayWithOffset class layout. struct ArrayWithOffsetData { diff --git a/src/coreclr/tests/src/Interop/StringMarshalling/UTF8/UTF8Test.cs b/src/coreclr/tests/src/Interop/StringMarshalling/UTF8/UTF8Test.cs index a8f58b3efa6f8bba5ab96e123398f95aa8a248f7..7bfe19fdac5117194a0ef2906409390124d45ebd 100644 GIT binary patch literal 9308 zcmds6TW{RP6@K4eG1(^y*-~D8_nm^dsI*SsXL%Fs#_ z1mFuhB<>G*$YZ~~+-Q*27dO|QVU2d+C5d?6NMo?ybUQ>RJP zG_akA$HrxWL5*!4nh zmv=Vzd)z&9p142`wV>C(1oN^a1(f#fYTCm?@^IZrt@&M8=bT$qIg z!@bawAkw`!y2?U+m91WD^BzTq)0`yANoMhRO7W@a+*E!c=mWv>KSlXiezeS~;jZ_vCNvY!U3w%dJS&iHm^;4ZN z>gPYrE=iu}-s-&XdTCr+F?2V~55zFCmKL-PcQ^g11gD^YJB zVL%6n9CVEL+|6iZ&x=pTvA6Fmlnw^%lOj4CQot;%5fzH_69@;TO4mz)y4IHYvER=Yc`hI}Yd8|f!R*sW+ z$5blXb{WrkN`gvVGgtX1?WllQ{VZ6j0;^K!VAttLvY3$;Z5hlNsfCg|z#?*N>@i1I zMUvrBMK1P&NyVb-tI}b!N$$>qyZYn47bI2wbMKWI=KC7OAXS9)cZ@~hyrD=tYzsfN zdJxOEa|9vqt)*ro%pHQ*@rgXgFUeia+Fs9V`{}+34U3aiu-6Y%_Y4nr)4j*WD_PG( zA^JSU;jsZ*kRC(KPPO()Z9833n7(de#upa-kM%6A4gV(`?Rt{pX#L)a@6KwnZI@eb zCe$v7Zr-WcJWFZ|sT!-2Wd3wX*8#evsOCQ-t@wER& znDTX#x&4vK{?JeNNcA-`no}5o_0t_mp{#F%85DKFWgA#+gsp8tA*;d|Nww~?c`4hg zBDt7zO3tTSM!7vfwzSNA_Wd#gm69?3^wZOhaU~DLb&YcXRuXf|-!8mGM{#M1xLE?Z zZ&Roa3ZS+%iIW0#uN z1*Y<;;VQhU*^p;VZmiW`CGy-Yj9E^7bV*C*e8R_t9hT+7`XWM4>vB3NG|^+0e$wD@ z6Uiv#w1KU7*9av?=&pt|q(74orb$7qpvJKz(R&bEhMafFYhTt~p~?~yBqJ_zo9Q+B zEGp;NvO9!;*NvD|Elc;gD^VBG$DfZoe&_{O@zN64#%|%@WJ_#`?FGv7OVc*p#M{$x zk#+6SiCBnl3R4?Rm_c6h)4o%=d~q?wi$X{yI8Z8QXC&<8!UEE7!~6lg=64lg{s7a5 z0-8W-DfjPRzWgY=|HX|zKD_(FmoMG>@|En-Pk;UJ?u!pU%O8FE!NdEn)XO+Lw*+@6Ph#S?1rF~EJKz^OK&aa^ z_Teb|lOUo2JYa}a#XndJu`ypl_=|Wmh9)xD8w6wzx#m`d@v$z_T)0O3-&O_D$i%wz5*Ni5~UB!qz?c%T_MfrW(pp1=g5(nzkBA`l6LB_yl^ z8|fSTzAlGI4-A7&a-w0w4WeQdUO`{5{f$PUwIG2lK>`Fa4u%j(x5SGPK}j*&hd_b| zK$2VmakEuxM-1}8$87Mo!M&rGj$f4O>9uQ$S^5Go!J54@mAv1zD_)X*wGR8{>4X!*W-d@|?G+?gHIU1LX)<=vS( z_nv!xe)rrn*Z%mUQ|^>Y-7Bu+mfec$x&c0~pk?Sb{pULFQ`dGg?qxTJtMh2zLcc3+ z5q&zi>!MX!R$L#~Y~h$qf4AYkdB5n^(6WM`4g7X7dH}jlxle-w@-XM>&2=r{b%2pW zjO>6~>Q?c6&Cf3U-^A=&Xt(^aH~G2fE_jJTdX}SE^kYvQ@d`$qgC1twEI#dQ9j^uL z*Mpp!?t2)!jIpV|+w@nf1+8$Tv~C!U3K!JP7UW`|HPD%LH*q%v-3@ofYp(2R+r8># zriG~OHaMroX`xNm10`Nv$5UE;Fp}CFdfmYIQL~-{^=sY&lw7=nR{l1|(<}OZMxjFK zGV8wvTby^#LrS(SgT^BGqD}S5EGTk3bwPfpiGlBZ%YUZm%b$7olIP(9B=32B4_(Vm zJ))yyXZH1d9kiz0G)A673%Qp$C`%VQ5?@tM|7NXzZP2=ck(A@4TgdUpC>X*%ebE0p z=v~CTeaN!z=Rb?97P#$#|3!Qh%V+ zDqf^kuafvWEZM<0VvP7rCP}r#t(<;{X={O&sDT)F^kU^L^iORK(k-BZE_N+2KRMT<$D^DGuX$t>* z2ESDtQLAQmtheiZqI9q_(Ei=s~ciPxlGS^PswDe zw)j*bf|T#=Kvuc4(m%o16!x10zGm2Z95GdRuJ+}}1y8GEi`Y_zVs&u4JjO}6TEp4O z)+*&;l1N&mnI|}UoN%;0#%1|cwQs(Pm2cH#kmJc0H+@E$&txZ)H|FJI#`Yreaz*X| z*7Yo#E@o{SdHGz{`b9jekUer-Ry$f(@sN`&D=21yl408ZZR9E3I}IUO9Ix_v2=@u0 zot5V*9#k*&a=W5lQ{;Q2GP=p;aJsX|YbNxN)bIDIG@+7E#?=7pAK#2^Qb@yH>Uc1Q zJ9h3XY?Sa${Wc%ZVboN~mtp5950a=lybMaBj8%{xa6nK(_c2xvt;p`Qjh*%;Y9Op= z^xawf5+QD4x5FGD)Q%!nK5}B1(gGc^L=gkj7M~PDLb;C)YFM48z-LxGSs@x?N4bt& zWSL$^?dR%wXSO0(JE_mou67lQuVHp%bq&DEYF8uL_;jhH)%0U)^k~|my=)gO>#)`& zLqu3YCc^qNJ+SZfF{+OFVlYCrc^s>nRbIsPZs>KwierX&)?T3|s=Z@!wa0RfCWZFd zeyx1&qBioYKt7|ev=kTfYt+HwHMCEx4wdi0d`#C1%TL&^1-}TZl(ugfrrACry`ru% z4aX{i;b_%WxQ19w>1vhL8H>C1kfyAKmSdtNk>~q}e!g*Fy}w!uA-XCLag^1k_3Mv{ zJDH{|x1lse{T^0TQsvTbYn2S2_32TsQcfB#b+wxx`}>)4-x7Jc$?@KJJs zL9pjv3Rd(Kx!Izx!!08-;C|YAt@>)YWHFrW09Dseb+EZcn4Oo?tHv3=S>)T!HE_CK zE6WPGDB3~2u!xZ{DaM1oG0MjWgAeZO#NW^d^R*DxWUWu6|HE#<`t4E2)_K269Pb*Kq%Oc+mLtv`HLj5XE=ROS|V9q$`+OTC=q zQMhZVXtYl{FBLoxTa|NON1tI_%Sa%yK%+A(f5O3wqicou5tPpk0k(U|~O<1~tB7mms- zrsI$L8dlU%*$J+!o}&9WYw7R$ymKAr#l#D!U-3VlF)Mlo&q}h@(#Xd~6>mjuv{q&* z#{D{6J?62ZDmUuA;w1w_tM#k62Ukg_^fX3nts*v#JND3{)Q|K;r(JPntvQVmJR6p} zZ_(a!_9l z`4zjB+2oUBHYMRX;x)g@@CX}cHO?|^W>?i`;W7`>c-I;8oW!G~_+Cj-NWU#7+ICQ_Xq7t8lkytV zq|PC$n&79NA727iEW4{fan+o1R?I`f7^Kf<>wKoYIyKE$|JX z8C%Qy)@1l9wd9oPV!FjW^zBdm!$4rxZ6v1@p%`mJIQpUC3SBn?_$Ke z=>IqLeJiQ=BcIW=Qeu3(fDoSca5Jn#>Yh$=)=D)8wbv9@9(WF zVpbtM*C^r|b1YtmnSx+GSMN2TUbwQx(O#-vx+;b1j#j<&jE20=;7&figUTcPVeY4O zZq4&%p(WjMsC4}6MZLZN-4)-@#InOJP|9jETvIo?OSTjuU`dn9W1iyM3>ch_>S!; zVc9r5E|gADZ8XA#<0g-Hbu9A#M8CtaMG+GZ#||1{@pXOR*GR3K^ZzZ#+k|kx8OJ>t z6|eMUt89psRb$G~VD#F`nu4wG#&AOU$XA8x@?URU_GTf*Lp~FD5*hSA`|h?}BW2zp z?>OpaVV1r8-O%9ckPB E7vQ9MX8-^I diff --git a/src/coreclr/tests/src/Interop/StringMarshalling/UTF8/UTF8TestNative.cpp b/src/coreclr/tests/src/Interop/StringMarshalling/UTF8/UTF8TestNative.cpp index 9759c1a..77e4b9c 100644 --- a/src/coreclr/tests/src/Interop/StringMarshalling/UTF8/UTF8TestNative.cpp +++ b/src/coreclr/tests/src/Interop/StringMarshalling/UTF8/UTF8TestNative.cpp @@ -99,7 +99,7 @@ LPSTR build_return_string(const char* pReturn) // end up with different byte sequence. const int NSTRINGS = 6; -#ifdef _WIN32 +#ifdef _WIN32 wchar_t *utf8Strings[] = { L"Managed", L"S\x00EEne kl\x00E2wen durh die wolken sint geslagen" , L"\x0915\x093E\x091A\x0902 \x0936\x0915\x094D\x0928\x094B\x092E\x094D\x092F\x0924\x094D\x0924\x0941\x092E\x094D \x0964 \x0928\x094B\x092A\x0939\x093F\x0928\x0938\x094D\x0924\x093F \x092E\x093E\x092E\x094D", @@ -157,7 +157,7 @@ extern "C" DLL_EXPORT void __cdecl StringBuilderParameterInOut(/*[In,Out] String } //out string builder -extern "C" DLL_EXPORT void __cdecl StringBuilderParameterOut(/*[In,Out] StringBuilder*/ char *s, int index) +extern "C" DLL_EXPORT void __cdecl StringBuilderParameterOut(/*[Out] StringBuilder*/ char *s, int index) { #ifdef _WIN32 @@ -284,6 +284,10 @@ extern "C" DLL_EXPORT void __cdecl StringParameterRef(/*ref*/ char **s, int inde } } + if (*s) + { + CoTaskMemFree(*s); + } // overwrite the orginal *s = (LPSTR)(CoTaskMemAlloc(sizeof(char)* (strLength + 1))); memcpy(*s, pszTextutf8, strLength); @@ -293,15 +297,6 @@ extern "C" DLL_EXPORT void __cdecl StringParameterRef(/*ref*/ char **s, int inde #endif } -extern "C" DLL_EXPORT void __cdecl StringParameterLPStr(/*out*/ char **s) -{ - const char *managed = "ManagedString"; - size_t strLength = strlen(managed); - *s = (LPSTR)(CoTaskMemAlloc(sizeof(char)* (strLength + 1))); - memcpy(*s, managed, strLength); - (*s)[strLength] = '\0'; -} - // delegate test typedef void (__cdecl * Callback)(char *text, int index); extern "C" DLL_EXPORT void _cdecl Utf8DelegateAsParameter(Callback managedCallback) -- 2.7.4