Bug 238001 – [WebGPU] Remove the double pointer indirection in front of all objects

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[WebGPU] Remove the double pointer indirection in front of all objects

[Myles C. Maxfield]

Created attachment 454933 [details]
Patch

[Myles C. Maxfield]

*** Bug 237921 has been marked as a duplicate of this bug. ***

[Kimmo Kinnunen]

Comment on attachment 454933 [details]
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).beginComputePass(*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(*descriptor));


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

[Kimmo Kinnunen]

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

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

So you have plain-old opaque struct api:

typedef struct WGPUBuffer WGPUBuffer; 
// struct WGPUBuffer opaque, never defined.
class WebGPU::Buffer {};

WGPUBuffer* releaseToAPI(RefPtr<WebGPU::Buffer>&& e)
{
     return reinterpret_cast<WGPUBuffer*>(e.leakRef());
}


WebGPU::Buffer& fromAPI(WGPUBuffer* b)
{
   return *reinterpret_cast<WebGPU::Buffer*>(b);
}

Strictly that should be UB too, but at least that's just something everybody lives with, I suppose.

[Myles C. Maxfield]

Created attachment 455317 [details]
Patch

[EWS]

Committed r291687 (248729@main): <https://commits.webkit.org/248729@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455317 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90651490>