[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 at pmr@std@@")
> +DEFINE_RTTI_DATA1(aligned_resource, 0, &base_memory_resource_rtti_base_descriptor, ".?AV_Aligned_new_delete_resource at pmr@std@@")
> +DEFINE_RTTI_DATA1(unaligned_resource, 0, &base_memory_resource_rtti_base_descriptor, ".?AV_Unligned_new_delete_resource at pmr@std@@")
> +DEFINE_RTTI_DATA1(null_resource, 0, &base_memory_resource_rtti_base_descriptor, ".?AV_null_resource at 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 at std@@YAXXZ");
> +    if (!throw_bad_alloc) FIXME("Failed to get address of ?_Xbad_alloc at 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