Re: [PATCH 2/2] msvcp140_1: Implement the DLL.

[Piotr Caban]

Hi Arek,

I still need to look more into first patch so you can wait with resend. 
Here are some initial comments:

On 4/1/21 3:38 PM, Arkadiusz Hiler wrote:
> +#ifdef __i386__
> +#define MAX_UNALIGNED_ALIGNMENT 8
> +#else
> +#define MAX_UNALIGNED_ALIGNMENT 16
> +#endif

It probably corresponds to operator new alignment. If so it would make 
more sense to define it as:
#define NEW_ALIGNMENT (2*sizeof(void*))
so it matches with new alignment on arm.
> +DEFINE_RTTI_DATA0(base_memory_resource, 0, ".?AVmemory_resource@pmr@std@@")
> +DEFINE_RTTI_DATA1(aligned_resource, 0, &base_memory_resource_rtti_base_descriptor, ".?AV_Aligned_new_delete_resource@pmr@std@@")
> +DEFINE_RTTI_DATA1(unaligned_resource, 0, &base_memory_resource_rtti_base_descriptor, ".?AV_Unligned_new_delete_resource@pmr@std@@")
> +DEFINE_RTTI_DATA1(null_resource, 0, &base_memory_resource_rtti_base_descriptor, ".?AV_null_resource@pmr@std@@")

This doesn't match with native RTTI data. If you need help with that 
part please let me know
> +DEFINE_THISCALL_WRAPPER(aligned_do_allocate, 12)
> +void* __thiscall aligned_do_allocate(memory_resource *this, size_t bytes, size_t alignment)
> +{
> - return MSVCRT_operator_new_aligned(bytes, alignment);
>
> +}

This doesn't match the tests that are calling free (instead of 
_aligned_free) in some cases.
> +DEFINE_THISCALL_WRAPPER(aligned_do_deallocate, 16)
> +void __thiscall aligned_do_deallocate(memory_resource *this, void *p, size_t bytes, size_t alignment)
> +{
> - MSVCRT_operator_delete_aligned(p, alignment);
>
> +}

Same here.
> +static memory_resource impl_aligned_resource = { &MSVCP_aligned_resource_vtable };
> +static memory_resource impl_unaligned_resource = { &MSVCP_unaligned_resource_vtable };
> +static memory_resource impl_null_resource = { &MSVCP_null_resource_vtable };
> -
>
> +memory_resource *default_unaligned_resource = &impl_unaligned_resource;
> +memory_resource *default_aligned_resource = &impl_aligned_resource;

Quick test shows that _Aligned_get_default_resource and 
_Unaligned_get_default_resource returns the same data after 
_Aligned_set_default_resource(0xdeadbeef) call. Probably there's only 
one default_resource variable that is shared in both of the functions. 
It should probably return default object when set to NULL and stored 
object otherwise. I'm thinking about something like:
memory_resource* __cdecl _Aligned_new_delete_resource(void)
{
     static memory_resource impl_aligned_resource = { 
&MSVCP_aligned_resource_vtable };
     return &impl_aligned_resource;
}

memory_resource* __cdecl _Aligned_get_default_resource(void)
{
     if (default_resource) return default_resource;
     return _Aligned_new_delete_resource();
}

memory_resource* __cdecl _Aligned_set_default_resource(memory_resource *res)
{
     memory_resource *ret = 
InterlockedExchangePointer((void**)&default_resource, res);
     if (!ret) ret = _Aligned_new_delete_resource();
     return ret;
}
> +static void init_cxx_funcs(void)
> +{
> - msvcp140 = LoadLibraryA("msvcp140.dll");
> - if (!msvcp140) FIXME("Failed to load msvcp140.dll\n");
> - throw_bad_alloc = (void*)GetProcAddress(msvcp140, "?_Xbad_alloc@std@@YAXXZ");
> - if (!throw_bad_alloc) FIXME("Failed to get address of ?_Xbad_alloc@std@@YAXXZ\n");
>
> +}

Please return error (so dll fails to load) if msvcp140 can't be loaded.
> +static void test__Aligned_new_delete_resource(void)
> +{
> - void *ptr;
> - memory_resource *resource = p__Aligned_new_delete_resource();
> - ok(resource != NULL, "Failed to get aligned new delete memory resource.\n");
> -
> - /* calling dtor should be harmless nop */
> - call_func1(resource->vtbl->dtor, resource);

Please avoid adding such tests (or add them as last test in the file). 
Sometimes it changes object behavior in unexpected ways. I guess that in 
this case it should work but it's better to play safe.
> - p_free(ptr); /* aligned delete, crashes with non-aligned */

Looks like the comment was copied from _aligned_free case.
> +static void test_get_set_defult_resource(memory_resource *(__cdecl *new_delete_resource)(void),
> -
>                                     memory_resource *(__cdecl *get_default_resource)(void),
> -
>                                     memory_resource *(__cdecl *set_default_resource)(memory_resource *resource))
>
> +{
> - memory_resource *new_resource = new_delete_resource();
> - memory_resource *default_resource = get_default_resource();
> - ok(default_resource == new_resource, "Expected the default memory resource to be equal new/delete one.\n");
> -
> - default_resource = set_default_resource((void*)0xdeadbeef);
> - ok(default_resource == new_resource, "Expected that setting default resource would return the old one.\n");
> -
> - default_resource = get_default_resource();
> - ok(default_resource == (void*)0xdeadbeef, "Expected that setting reasource would take effect.\n");
> -
> - default_resource = set_default_resource(NULL);
> - ok(default_resource == (void*)0xdeadbeef, "Expected that setting default resource would return the old one.\n");
> -
> - default_resource = get_default_resource();
> - ok(default_resource == new_resource, "Expected that setting default resource to NULL would reset the value.\n");
>
> +}

Please add a test that calls p__Unaligned_get_default_resource after 
p__Aligned_set_default_resource(0xdeadbeef) call.

Thanks,
Piotr