Ownership of returned strings

[kvark]

Related to #82

We had a small discussion about wgpuAdapterGetProperties. It needs to return a name and some description. When are these going to be freed?

It seems to me that we should make the storage totally owned by the user. So they'd provide a pointer and the capacity, and we'd just fill out the storage with some chars.

[kainino0x]

I would probably just have them owned by the WGPUAdapter. Though, if doing that, the whole WGPUAdapterProperties could just be owned by the WGPUAdapter and you wouldn't need this writable pointer thing at all.

[kvark]

Ok, right, that's an option. It's just not super convenient for us, because it's a C string, and our adapters don't like holding onto those kind.

[kainino0x]

We discussed a proposal in today's meeting and I think we were pretty happy with it:

typedef struct WGPUAdapterPropertiesImpl* WGPUAdapterProperties;

void wgpuAdapterPropertiesReference(WGPUAdapterProperties);
void wgpuAdapterPropertiesRelease(WGPUAdapterProperties);

WGPUAdapterProperties wgpuAdapterGetProperties(WGPUAdapter);

typedef struct WGPUAdapterPropertiesV1 {
    // (no chained struct here)
    uint32_t vendorID;
    char const * vendorName;
    char const * architecture;
    uint32_t deviceID;
    char const * name;
    char const * driverDescription;
    WGPUAdapterType adapterType;
    WGPUBackendType backendType;
} WGPUAdapterPropertiesV1;

typedef struct WGPUAdapterPropertiesDAWN {
    // example extension
} WGPUAdapterPropertiesDAWN;

// The structs are owned by AdapterProperties, returned by reference, freed when it's freed
WGPUAdapterPropertiesV1 const * wgpuAdapterPropertiesGetV1(WGPUAdapterProperties);
WGPUAdapterPropertiesDAWN const * wgpuAdapterPropertiesGetDAWN(WGPUAdapterProperties);
// Extensions can also return things other than structs
int wgpuAdapterPropertiesGetFavoriteNumber(WGPUAdapterProperties);
WGPUSurface

This is a bit of a different paradigm than we use for input structs. It's a bit more like NXT's original design where input structs were all builders instead of using chained structs. But I like it a lot more than the Vulkan style where you have to call twice - once to ask for the length, and one to get the value. It was raised in the meeting that that style is especially painful when you're returning something that's generated on the fly (like some strings) and you have to generate the exact same string twice or somehow cache it.

It's a little heavyweight with the refcounting, but anyway right now we only have a few cases like this (AdapterProperties, SupportedLimits/EnumerateFeatures, ?) so I think it's fine.

[austinEng]

I think I'm neutral about it because the extra object still feels unnecessary to me:

• For the adapter, the extra object seems unnecessary to me because intuitively, the properties of the adapter can live as long as the adapter they came from.
• For limits, I don't think we don't have to use this separate object pattern because all the limits are numeric - there's nothing variable-length there
• For EnumerateFeatures you can call it twice which is a bit ugly but is cheap for the implementation to do. It doesn't apply in all cases. @cwfitzgerald mentioned there are some strings that are returned for which it would be unfortunate to generate them twice - are there examples of wgpu's extensions that do this to look at?

[kainino0x]

In last thursday's meeting we settled on the following proposal (IIRC - it's been a few days):

• have the adapter strings be owned by the caller
• add a void wgpuAdapterPropertiesFreeMembers(WGPUAdapterProperties) which frees anything in the adapter properties struct (and chained structs) that needs to be freed. It was proposed to take the properties struct by value to make it clear that it's not going to free the struct itself (or its chain), but I also think a pointer would be OK.
• keep the double-call (size-then-data) pattern for EnumerateFeatures
• for GetMappedRange (see #194 #64), IIRC we proposed to let the caller optionally choose to receive an error message in string form, something like the following. If they don't pass it they can just know there was an error because they got nullptr (like today).

struct WGPUGetMappedRangeError {
    char const * errorMessage;
}

void * wgpuBufferGetMappedRange(WGPUBuffer buffer, size_t offset, size_t size, WGPUGetMappedRangeError * Error WGPU_NULLABLE);

void wgpuGetMappedRangeResultFreeMembers(WGPUGetMappedRangeError);

• For new things that come up, be sure to avoid the double-call pattern for getting any data that can change length over time, because it will cause TOCTOU-like issues.

Also considered and rejected:

• Have adapter strings be owned by the adapter. This causes problems with aliasing especially in Rust because it holds pointers into the Adapter object, which then can't be used for anything afterward because it's borrowed. Safe wgpu could avoid this by returning copies of the strings in owning containers, but
• Double call pattern for AdapterProperties, but if you want you can just make a single call and we'll chop off any strings that are too long. We provide suggested lengths for each of the fields so you can avoid this "usually".
• A generic wgpuReturnedDataFree(void*) that works on specific strings and arrays returned by the library. Annoying because you have to call it on a bunch of things individually, and also users may be confused about whether they should use this for regular WGPU objects like WGPUTexture.
• Let the caller provide a pointer to a malloc function on the Instance and make them free everything themselves. Connor says this runs into issues where different DLLs on Windows can have distinct heaps so this isn't actually guaranteed to work (because malloc is called from the DLL binary but free is called from the application binary)
• Pass a callback that is called inline, and free the stuff after the callback is done. Kludgy, and painful in C.

[kainino0x]

tentatively marking as "has resolution"

[Kangz]

This seems like it could work, it's a bunch of work to implement and makes the C++ wrapper harder to generate, but is otherwise doable. We also need to handle extensible out structures this way.

[austinEng]

What should the behavior be if you write to an output struct multiple times?

WGPUAdapterProperties properties{};

wgpuAdapterGetProperties(adapter, &properties);
wgpuAdapterGetProperties(adapter, &properties);
wgpuAdapterGetProperties(adapter, &properties);
wgpuAdapterGetProperties(adapter, &properties);

Each call will allocate strings for properties that must be freed with wgpuAdapterPropertiesFreeMembers(&properties);

I see two possibilties:

1. successive calls like this will implicitly perform the call to wgpuAdapterPropertiesFreeMembers for you if the members are non-null - assuming that the developer did not free it.
2. successive calls like this are the developer's problem. They should be performing a free in-between each.

(1) might be surprising for users of the C API, and would also result in invalid frees if the struct isn't zero initialized. Maybe that's fine though because it's C. High-level language bindings will zero init for you. I believe this approach also requires FreeMembers to null out fields so that we don't double-free.

I think (2) would be OK so long as:

• we can have it so that higher-level language bindings free for you. In practice, this means FreeMembers needs to take the argument by mutable pointer so it can null things out and know when it should be freeing. GetProperties might be implemented like:

  void Adapter::GetProperties(wgpu::AdapterProperties* properties) {
   *properties = {};
   wgpuGetAdapterProperties(cAdapter, reinterpret_cast<WGPUAdapterProperties*>(properties);
  }
  
  AdapterProperties::~AdapterProperties() {
    if (name != nullptr && driverDescription != nullptr ...) {
       wgpuAdapterPropertiesFreeMembers(this);
      name = nullptr;
      driverDescription = nullptr;
      // etc...
    }
  }

  although, this is unfortunate because now the C/C++ semantics don't match. It could be better to make it so the C API says that FreeMembers nulls out all the fields, and before writing to any output struct, the C API nulls out all fields

[kainino0x]

I definitely prefer option 2. There does need to be a valid empty state for wgpu::AdapterProperties so you can create it before passing it to GetProperties. But I'm not sure the destructor actually needs to set it to the empty state in C++, since it's about to get freed? I think I don't quite understand the problem.

The chained structs make things a little messy, because even in C++ you have to make sure you don't touch the chain before freeing. Assorted thoughts:

• Top struct is RAII so can't be copyable
• Chained structs are pointed-to so should be non-movable at least while they're populated. They could also ASSERT they've been cleared out in their destructors.
• If we somehow wrapped the whole chain into a single RAII object it could be simpler....

[kainino0x]

Note that with #266, wgpuAdapterPropertiesFreeMembers will be replaced with wgpuAdapterInfoFreeMembers.