Fix ber scanf/printf, fix for a reverted PR. #52178
Conversation
Use ber_put_*/ber_get_* for Unix
|
/azp run runtime-libraries-coreclr outerloop-osx, runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
The tests involving the new code have passed on windows x64/linux x64/linux arm64/MacOS x64/MacOS arm64. |
|
I would like to merge this PR this week if possible, could somebody please take a look? @AaronRobinsonMSFT, @jkoritzinsky |
| @@ -15,19 +15,19 @@ internal static class BerPal | |||
|
|
|||
| internal static int FlattenBerElement(SafeBerHandle berElement, ref IntPtr flattenptr) => Interop.Ldap.ber_flatten(berElement, ref flattenptr); | |||
|
|
|||
| internal static int PrintBerArray(SafeBerHandle berElement, string format, IntPtr value, int tag) => Interop.Ldap.ber_printf(berElement, format, __arglist(value)); | |||
| internal static int PrintBerArray(SafeBerHandle berElement, string format, IntPtr value, nuint tag) => Interop.Ldap.ber_printf(berElement, format, __arglist(value)); | |||
There was a problem hiding this comment.
Do we want nuint on Windows? If the type is defined to be C unsigned long then it should be uint since on Windows a C unsigned long is 32-bits
There was a problem hiding this comment.
Oops. I misread the comments in the PR. Let's change tag to _ since it isn't used.
There was a problem hiding this comment.
Can I have several args named "_", like:
public static int ber_printf_tag(SafeBerHandle _, string _, nuint _)
?
|
|
||
| internal static int ScanNext(SafeBerHandle berElement, string format) => Interop.Ldap.ber_scanf(berElement, format); | ||
| internal static int PrintTag(SafeBerHandle berElement, string format, nuint tag) => Interop.Ldap.ber_printf(berElement, format, __arglist(tag)); |
There was a problem hiding this comment.
Is it supposed to be used here? Cast to uint?
There was a problem hiding this comment.
you are right, fixed, don't understand why x86 Libraries tests passed with this.
|
/azp run runtime-libraries-coreclr outerloop-osx, runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
runtime-libraries-coreclr outerloop had 32 failures, runtime-libraries-coreclr outerloop-osx had 8 failures. They all were marked as new but looked unrelated but I did not see some of them in the previous so I decided to double-check and triggered the main testing and it showed the same results without my changes runtime-libraries-coreclr outerloop 38 failures, runtime-libraries-coreclr outerloop-osx 6 failures and the set was ~the same. |
ghost
locked as resolved and limited conversation to collaborators
#51205 was reverted because of the failures on arm64 Linux.
@jkoritzinsky, @AaronRobinsonMSFT PTAL at the last commit here. Thanks for pointing me to https://docs.microsoft.com/en-us/dotnet/standard/native-interop/best-practices#cc-long, I have tried to use
CULongbut it is supported only by 6.0, so I would have to deletenetcoreapp2.0andnetstandard2.0supportfrom
runtime/src/libraries/System.DirectoryServices.Protocols/src/System.DirectoryServices.Protocols.csproj
Line 5 in d2fba8f
and
runtime/src/libraries/System.DirectoryServices.AccountManagement/src/System.DirectoryServices.AccountManagement.csproj
Line 6 in d2fba8f
and I guess it is not desired?
The fix was to use nuint instead of unit so the default tag value is passed as
0xffffffffffffffffon Linux. This parameter is not used for Windows, so we don't care about it there.Fixes #49105.