[webkit-reviews] review canceled: [Bug 238001] [WebGPU] Remove the double pointer indirection in front of all objects : [Attachment 454933] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has canceled Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 238001: [WebGPU] Remove the double pointer indirection in front of all
objects
https://bugs.webkit.org/show_bug.cgi?id=238001

Attachment 454933: Patch

https://bugs.webkit.org/attachment.cgi?id=454933&action=review




--- Comment #3 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 454933
  --> https://bugs.webkit.org/attachment.cgi?id=454933
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454933&action=review

> Source/WebGPU/ChangeLog:18
> +

Unfortunately you have to invert the relationship.

Now you have reasoning of:
"WGPUBufferImpl has the same layout as WebGPU::Buffer. This means I can take
hunk of data pointed by WGPUBufferImpl* and treat it as WebGPU::Buffer".

This is invalid reasoning. You can imagine a compiler that knows everything. It
will know that the program never creates a single WGPUBufferImpl. This means it
will remove all code that accesses data through WGPUBufferImpls, as that simply
cannot happen, as there are never instances of such.

So when you have code like:
void wgpuAdapterRelease(WGPUAdapter adapter)
{
    adapter->deref();
}

The code will be removed, because WGPUAdapterImpl is never instantiated, so
that ->deref() call cannot ever be made.


This is the same scenario as having:
struct Kimmo {
  int a;
};
struct Myles {
  int a;
}

Myles* m = getMyles();

Kimmo* k = static_cast<Kimmo*>(m);
printf("%d", k->a);



struct WGPUBufferImpl : public WebGPU::Buffer;

// This is UB, WebGPU::Buffer is not WGPUBufferImpl.
WGPUBufferImpl* wgpuobj = static_cast<WGPUBufferImp *>(new WebGPU::Buffer());

// This is UB, WebGPU::Buffer is not float.
float* wgpuobj = static_cast<float*>(new WebGPU::Buffer());


E.g. you have to have something where WGPUBufferImpl is instantiated.

Since you instantiate WebGPU::Buffer instances, you need to have

struct WGPUBufferImpl {};
class WebGPU::Buffer : public WGPUBufferImpl {};

This way you can downcast:

WGPUBufferImpl* wgpuobj = new  WebGPU::Buffer();

// This is defined, since wgpuobject points to a real WebGPU::Buffer.
static_cast<WebGPU::Buffer*>(wgpuobj)->doSomethingBuffery();

So going back:

WebGPU::Adapter* fromAPI(WGPUAdapter adapter)
{
    // This static cast is DEFINED, IFF adapter was created from a pointer that
pointed to WebGPU::Adapter. 
    return static_cast<WebGPU::Adapter*>(adapter);
}

void wgpuAdapterRelease(WGPUAdapter adapter)
{
    fromAPI(adapter)->deref();
}



The minimal type punning is achieved when:
-Input downcasts
- Output is natural subtype conversion

if you:
-  don't want empty base clasess
- void* types in the api

Then you need to type pun to opaque struct ptrs in the output.

> Source/WebGPU/WebGPU/CommandEncoder.mm:363
> +    return
static_cast<WGPUComputePassEncoder>(WebGPU::fromAPI(commandEncoder).beginComput
ePass(*descriptor).leakRef());

So this is incorrect type punning, leading to UB.

There is never an instance of WGPUComputePassEncoder in this program. So the
(all knowing) compiler knows this and can remove all code that ever references
these codes, as they never can execute. If they would execute, they'd be UB. 

You have to invert the inheritance, unfortunately.


so you write this code as

    return
WebGPU::releaseToAPI(WebGPU::fromAPI(commandEncoder).beginComputePass(*descript
or));


WGPUComputePassEncoder releaseToAPI(RefPtr<WebGPU::ComputePassEncoder>&& e)
{
     return e.leakRef();
}