On 01.12.2014 11:27, Martin Storsjo wrote:
- buf = malloc(sizeof(*buf));
- if (!buf)
return E_OUTOFMEMORY;- buf->buffer = malloc((length + 1) * sizeof(*buf->buffer));
- if (!buf->buffer) {
free(buf);return E_OUTOFMEMORY;- }
Some of your previous patches also contain changes to ./configure which is an autogenerated file. Afaik its not allowed to include that when submitting patches.
Feedback for this part:
* HSTRING and HSTRING_HEADER should probably be defined in some include fine. To ensure that the size of private structure matches you could also use C_ASSERT(...), and maybe even disable align of the structure fields.
* You should avoid using malloc(...) in Wine source
* http://msdn.microsoft.com/en-us/library/br224632(v=vs.85).aspx mentions: """Calling WindowsDeleteString decrements the reference count of the backing buffer, and if the reference count reaches 0, the Windows Runtime de-allocates the buffer."""
I don't see the concept of refcounted backing buffers anywhere in your code, which is most likely wrong. Its difficult to fix that later.
* I would suggest starting with some stub functions, and then add tests before sending the actual implementation.
BTW: Michael Müller was also working on some api-ms-win-core-* stubs, but I think he is also fine when your patches get upstream instead: https://github.com/wine-compholio/wine-staging/tree/master/patches/api-ms-wi...
Regards, Sebastian
Hi Sebastian,
On Mon, 1 Dec 2014, Sebastian Lackner wrote:
On 01.12.2014 11:27, Martin Storsjo wrote:
- buf = malloc(sizeof(*buf));
- if (!buf)
return E_OUTOFMEMORY;- buf->buffer = malloc((length + 1) * sizeof(*buf->buffer));
- if (!buf->buffer) {
free(buf);return E_OUTOFMEMORY;- }
Some of your previous patches also contain changes to ./configure which is an autogenerated file. Afaik its not allowed to include that when submitting patches.
Ok, I'll keep that in mind (and I can resubmit them without those changes if that's requested).
Feedback for this part:
- HSTRING and HSTRING_HEADER should probably be defined in some include
fine. To ensure that the size of private structure matches you could also use C_ASSERT(...), and maybe even disable align of the structure fields.
Right, they should probably go into hstring.h and winstring.h then. (One needs to take more care about the HSTRING_HEADER struct if making it public though, since the public definition should match the right size in the ABI and not have any public meaningful members, only placeholder to make it a certain size, while the internals of the DLL uses that to store certain fields.)
- You should avoid using malloc(...) in Wine source
Ok, I should have guessed that. Is it ok to use memcpy for copying data though?
"""Calling WindowsDeleteString decrements the reference count of the backing buffer, and if the reference count reaches 0, the Windows Runtime de-allocates the buffer."""
I don't see the concept of refcounted backing buffers anywhere in your code, which is most likely wrong. Its difficult to fix that later.
I had a really hard time understanding the MSDN part here - I don't find any corresponding function for increasing a refcount, so I don't see how the HSTRING itself would be refcounted. So I think that only matters once you pass those string handles into the full Windows Runtime itself.
I don't see how it would be hard to add that to the internal representation of HSTRING later though - as long as that representation is purely internal we should be able to change it. And if it needs to be synced with something else (e.g. Windows Runtime accessing data from within HSTRING directly without going through the functions here) we'd need to sync with some unofficial/unknown/unspecified ABI.
- I would suggest starting with some stub functions, and then add tests
before sending the actual implementation.
That sounds like a sensible approach, although it'll probably take some time before I've got time to do it that way. This patch is pretty much separate from the rest of this patchset anyway (and I've got much less concrete need for it, contrary to the other patches), so I don't mind if it's deferred/dropped for now, separate from the rest at least.
// Martin
Hi Martin,
On 01.12.2014 12:40, Martin Storsjö wrote:
- HSTRING and HSTRING_HEADER should probably be defined in some
include fine. To ensure that the size of private structure matches you could also use C_ASSERT(...), and maybe even disable align of the structure fields.
Right, they should probably go into hstring.h and winstring.h then. (One needs to take more care about the HSTRING_HEADER struct if making it public though, since the public definition should match the right size in the ABI and not have any public meaningful members, only placeholder to make it a certain size, while the internals of the DLL uses that to store certain fields.)
I know that this is a bit tricky. I would suggest to put the "dummy" version of HSTRING_HEADER into the include file, and then define a different structure inside of the implementation with C_ASSERTS(...) to ensure that the size between both matches. The code could then use something like this (similar to interfaces):
static inline struct hstring_private impl_from_HSTRING(HSTRING string) { return (struct hstring_private *)string; }
static inline struct hstring_private impl_from_HSTRING_HEADER(HSTRING_HEADER *header) { return (struct hstring_private *)header; }
... { struct hstring_private *s1 = impl_from_HSTRING_HEADER(first_string); struct hstring_private *s2 = impl_from_HSTRING(another_string); ...
}
- You should avoid using malloc(...) in Wine source
Ok, I should have guessed that. Is it ok to use memcpy for copying data though?
Yes, all other functions memcpy, memmove, ... are fine. For memory allocation you should always use the Heap functions (HeapAlloc, HeapFree, ...). Please note that calling HeapFree(...) with a null pointer is perfectly fine, so no additional if - checks are needed for that.
mentions: """Calling WindowsDeleteString decrements the reference count of the backing buffer, and if the reference count reaches 0, the Windows Runtime de-allocates the buffer."""
I don't see the concept of refcounted backing buffers anywhere in your code, which is most likely wrong. Its difficult to fix that later.
I had a really hard time understanding the MSDN part here - I don't find any corresponding function for increasing a refcount, so I don't see how the HSTRING itself would be refcounted. So I think that only matters once you pass those string handles into the full Windows Runtime itself.
I think the main purpose of that is to implement fast versions for functions like SubString(...). If its not created with WindowsCreateStringReference(...) both strings can use the same backing buffer, which is faster than copying part of the string.
Normally strings are immutable, but as it is also possible to get a raw pointer to the buffer ( http://msdn.microsoft.com/en-us/library/br224636(v=vs.85).aspx ) an application might depend on this specific implementation detail. Tests would really help here to confirm my theory that changing the substring also modifies the original one.
I don't see how it would be hard to add that to the internal representation of HSTRING later though - as long as that representation is purely internal we should be able to change it. And if it needs to be synced with something else (e.g. Windows Runtime accessing data from within HSTRING directly without going through the functions here) we'd need to sync with some unofficial/unknown/unspecified ABI.
Of course it is possible to change that, but when possible problems are known in advance it probably makes sense to think about that more carefully before including it. As already mentioned, tests could really be useful here to avoid later rewrites or issues in the implementation.
- I would suggest starting with some stub functions, and then add
tests before sending the actual implementation.
That sounds like a sensible approach, although it'll probably take some time before I've got time to do it that way. This patch is pretty much separate from the rest of this patchset anyway (and I've got much less concrete need for it, contrary to the other patches), so I don't mind if it's deferred/dropped for now, separate from the rest at least.
// Martin
Regards, Sebastian
Hi Sebastian,
On Mon, 1 Dec 2014, Sebastian Lackner wrote:
mentions: """Calling WindowsDeleteString decrements the reference count of the backing buffer, and if the reference count reaches 0, the Windows Runtime de-allocates the buffer."""
I don't see the concept of refcounted backing buffers anywhere in your code, which is most likely wrong. Its difficult to fix that later.
I had a really hard time understanding the MSDN part here - I don't find any corresponding function for increasing a refcount, so I don't see how the HSTRING itself would be refcounted. So I think that only matters once you pass those string handles into the full Windows Runtime itself.
I think the main purpose of that is to implement fast versions for functions like SubString(...). If its not created with WindowsCreateStringReference(...) both strings can use the same backing buffer, which is faster than copying part of the string.
Normally strings are immutable, but as it is also possible to get a raw pointer to the buffer ( http://msdn.microsoft.com/en-us/library/br224636(v=vs.85).aspx ) an application might depend on this specific implementation detail. Tests would really help here to confirm my theory that changing the substring also modifies the original one.
I did test this now, and no, a string created with WindowsSubstring doesn't share the same backing buffer as the original string it was based on (created with WindowsCreateString), so changes to either of them via the pointer from WindowsGetStringRawBuffer doesn't affect the other one.
So therefore I don't really see where reference counting would come into play with this API at all, unless the Windows Runtime hooks into HSTRING objects via some other (non-public) functions. Or is CloseHandle supposed to be able to handle HSTRINGs, and is there any corresponding function that would increase the reference count of any generic HANDLE?
// Martin
Hi Martin,
you are right that WindowsSubstring always creates a new backing buffer (although it is not necessary), but there is also WindowsDuplicateString. This function indeed uses the same backing buffer as you can verify with this small sample code: https://dl.dropboxusercontent.com/u/61413222/test-string.c
The MSDN also explicitly mentions that the refcount is incremented in this case: "If string was created by calling the WindowsCreateString function, the reference count of the backing buffer is incremented. If string was created by calling the WindowsCreateStringReference function, the Windows Runtime copies its source string to a new buffer and starts a reference count, which means that newString is not a fast-pass string." Source: http://msdn.microsoft.com/en-us/library/br224634(v=vs.85).aspx
It is necessary to implement reference counting in order to be fully compatible with the Windows implementation.
Regards, Michael
Am 06.12.2014 um 16:28 schrieb Martin Storsjö:
Hi Sebastian,
On Mon, 1 Dec 2014, Sebastian Lackner wrote:
mentions: """Calling WindowsDeleteString decrements the reference count of the backing buffer, and if the reference count reaches 0, the Windows Runtime de-allocates the buffer."""
I don't see the concept of refcounted backing buffers anywhere in your code, which is most likely wrong. Its difficult to fix that later.
I had a really hard time understanding the MSDN part here - I don't find any corresponding function for increasing a refcount, so I don't see how the HSTRING itself would be refcounted. So I think that only matters once you pass those string handles into the full Windows Runtime itself.
I think the main purpose of that is to implement fast versions for functions like SubString(...). If its not created with WindowsCreateStringReference(...) both strings can use the same backing buffer, which is faster than copying part of the string.
Normally strings are immutable, but as it is also possible to get a raw pointer to the buffer ( http://msdn.microsoft.com/en-us/library/br224636(v=vs.85).aspx ) an application might depend on this specific implementation detail. Tests would really help here to confirm my theory that changing the substring also modifies the original one.
I did test this now, and no, a string created with WindowsSubstring doesn't share the same backing buffer as the original string it was based on (created with WindowsCreateString), so changes to either of them via the pointer from WindowsGetStringRawBuffer doesn't affect the other one.
So therefore I don't really see where reference counting would come into play with this API at all, unless the Windows Runtime hooks into HSTRING objects via some other (non-public) functions. Or is CloseHandle supposed to be able to handle HSTRINGs, and is there any corresponding function that would increase the reference count of any generic HANDLE?
// Martin
Hi Michael,
On Sat, 6 Dec 2014, Michael Müller wrote:
you are right that WindowsSubstring always creates a new backing buffer (although it is not necessary), but there is also WindowsDuplicateString. This function indeed uses the same backing buffer as you can verify with this small sample code: https://dl.dropboxusercontent.com/u/61413222/test-string.c
The MSDN also explicitly mentions that the refcount is incremented in this case: "If string was created by calling the WindowsCreateString function, the reference count of the backing buffer is incremented. If string was created by calling the WindowsCreateStringReference function, the Windows Runtime copies its source string to a new buffer and starts a reference count, which means that newString is not a fast-pass string." Source: http://msdn.microsoft.com/en-us/library/br224634(v=vs.85).aspx
It is necessary to implement reference counting in order to be fully compatible with the Windows implementation.
Oh, I see - I hadn't checked this particular function (yet).
Ok then, then it's clear where/how to hook up the reference counting. I'll see if I can get to attempting implementing this sometimes soon.
// Martin
Hi Martin,
please note that besides functions like WindowsSubstring which implicitly manipulate the string buffer, there are also some functions for explicit string buffer management:
WindowsPreallocateStringBuffer - allocate a new string buffer with refcount 1 WindowsPromoteStringBuffer - create a new string based on an existing string buffer without incrementing refcount WindowsDeleteStringBuffer - decrement the refcount of a string buffer
Of course there are a lot more internal functions, but I think it should give a rough idea of how everything works on Windows. If you need any help with implementing this feel free to ask, we're also interested in this functionality. ;)
Regards, Sebastian
On 06.12.2014 17:26, Martin Storsjö wrote:
Hi Michael,
On Sat, 6 Dec 2014, Michael Müller wrote:
you are right that WindowsSubstring always creates a new backing buffer (although it is not necessary), but there is also WindowsDuplicateString. This function indeed uses the same backing buffer as you can verify with this small sample code: https://dl.dropboxusercontent.com/u/61413222/test-string.c
The MSDN also explicitly mentions that the refcount is incremented in this case: "If string was created by calling the WindowsCreateString function, the reference count of the backing buffer is incremented. If string was created by calling the WindowsCreateStringReference function, the Windows Runtime copies its source string to a new buffer and starts a reference count, which means that newString is not a fast-pass string." Source: http://msdn.microsoft.com/en-us/library/br224634(v=vs.85).aspx
It is necessary to implement reference counting in order to be fully compatible with the Windows implementation.
Oh, I see - I hadn't checked this particular function (yet).
Ok then, then it's clear where/how to hook up the reference counting. I'll see if I can get to attempting implementing this sometimes soon.
// Martin
On Sat, 6 Dec 2014, Sebastian Lackner wrote:
Hi Martin,
please note that besides functions like WindowsSubstring which implicitly manipulate the string buffer, there are also some functions for explicit string buffer management:
WindowsPreallocateStringBuffer - allocate a new string buffer with refcount 1 WindowsPromoteStringBuffer - create a new string based on an existing string buffer without incrementing refcount WindowsDeleteStringBuffer - decrement the refcount of a string buffer
Yep, I saw these as well. Although the MSDN page for WindowsDeleteStringBuffer doesn't talk about refcounts anywhere I can see.
Is the only way of using these string buffers to allocate a certain size, modify the WCHAR* that WindowsPreallocateStringBuffer returned, or are there any other functions involved with these as well?
// Martin
On 06.12.2014 17:48, Martin Storsjö wrote:
On Sat, 6 Dec 2014, Sebastian Lackner wrote:
Hi Martin,
please note that besides functions like WindowsSubstring which implicitly manipulate the string buffer, there are also some functions for explicit string buffer management:
WindowsPreallocateStringBuffer - allocate a new string buffer with refcount 1 WindowsPromoteStringBuffer - create a new string based on an existing string buffer without incrementing refcount WindowsDeleteStringBuffer - decrement the refcount of a string buffer
Yep, I saw these as well. Although the MSDN page for WindowsDeleteStringBuffer doesn't talk about refcounts anywhere I can see.
Is the only way of using these string buffers to allocate a certain size, modify the WCHAR* that WindowsPreallocateStringBuffer returned, or are there any other functions involved with these as well?
// Martin
No, there are no further functions involved. The idea behind this is that all WindowsStrings are immutable, and cannot (= should not) be modified anymore after a HSTRING was assigned. Allocating first the string buffer, and later promoting to a HSTRING allows to savely modify the contents (for example reading it from a file). I guess this is mainly for performance reasons, otherwise the content would always have to be copied.
On Sat, 6 Dec 2014, Sebastian Lackner wrote:
On 06.12.2014 17:48, Martin Storsjö wrote:
On Sat, 6 Dec 2014, Sebastian Lackner wrote:
Hi Martin,
please note that besides functions like WindowsSubstring which implicitly manipulate the string buffer, there are also some functions for explicit string buffer management:
WindowsPreallocateStringBuffer - allocate a new string buffer with refcount 1 WindowsPromoteStringBuffer - create a new string based on an existing string buffer without incrementing refcount WindowsDeleteStringBuffer - decrement the refcount of a string buffer
Yep, I saw these as well. Although the MSDN page for WindowsDeleteStringBuffer doesn't talk about refcounts anywhere I can see.
Is the only way of using these string buffers to allocate a certain size, modify the WCHAR* that WindowsPreallocateStringBuffer returned, or are there any other functions involved with these as well?
// Martin
No, there are no further functions involved. The idea behind this is that all WindowsStrings are immutable, and cannot (= should not) be modified anymore after a HSTRING was assigned. Allocating first the string buffer, and later promoting to a HSTRING allows to savely modify the contents (for example reading it from a file). I guess this is mainly for performance reasons, otherwise the content would always have to be copied.
Indeed, but since you don't seem to be able to change the length of the allocated HSTRING_BUFFER, I don't see much of the idea in that API (for e.g. creating strings dynamically), except for avoiding a copy when it is made into a HSTRING. But I guess that was an important enough usecase to warrant that API.
Anyway, I should be able to fit in support for these functions as well without any bigger issues.
// Martin
-
Martin Storsjö -
Michael Müller -
Sebastian Lackner