Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

Merged
merged 1 commit into from
Nov 12, 2020
Merged

Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

merged 1 commit into from
Nov 12, 2020

Conversation

MarijnS95
Copy link
Contributor

While this prefix may be an implementation detail the BSTR type guarantees to be able to store NULL values without accidentally truncating the string, hence its length must be available to replace the usual NULL terminator. This string length in bytes is stored in the four bytes preceding a BSTR pointer 1, whose value can be retrieved using SysStringLen.

This commit implements the type in the same way in WinAdapter such that users of the functions returning a BSTR (currently only in dxcisense) can expect and use the type in the same way on Linux as they would on Windows.
While this function and type currently only seem to be involved in names and logging where NULLs are unlikely, it is good practice to mirror the Windows type exactly for future-proofing.

Besides, SysAllocStringLen does not specify anything about taking ownership of strIn so the realloc here is wrong either way.

@MarijnS95
Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen
31ce626 
While this prefix may be an implementation detail the BSTR type
guarantees to be able to store NULL values without accidentally
truncating the string, hence its length must be available to replace the
usual NULL terminator.  This string length in bytes is stored in the
four bytes preceding a BSTR pointer [1], whose value can be retrieved
using SysStringLen.

This commit implements the type in the same way in WinAdapter such that
users of the functions returning a BSTR (currently only in dxcisense)
can expect and use the type in the same way on Linux as they would on
Windows.
While this function and type currently only seem to be involved in names
and logging where NULLs are unlikely, it is good practice to mirror the
Windows type exactly for future-proofing.

Besides, SysAllocStringLen does not specify anything about taking
ownership of strIn so the realloc here is wrong either way.

[1]: https://docs.microsoft.com/en-us/previous-versions/windows/desktop/automat/bstr
MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Nov 10, 2020
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit size
5cfd0f5 
Needs microsoft/DirectXShaderCompiler#3250
@AppVeyorBot
Copy link

✅ Build DirectXShaderCompiler 1.0.3852 completed (commit 07eee7a7a0 by @MarijnS95)

@ehsannas ehsannas self-requested a review November 12, 2020 16:43
@ehsannas ehsannas added the linux Linux-specific work label Nov 12, 2020
ehsannas
ehsannas approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @MarijnS95

@ehsannas ehsannas merged commit 2ade6f8 into microsoft:master Nov 12, 2020
@MarijnS95
Copy link
Contributor Author

@ehsannas Thanks for merging this quickly! Keep in mind that this is a slightly breaking change for Unix users that are free()ing the pointer currently. Following Traverse-Research/hassle-rs#10 we had a discussion whether DXC should export symbols for CoTaskMemFree (and SysFreeString, SysString*Len) so that we do not have to "guess" what primitives DXC uses to complement their Windows counterpart? What do you think?

On a similar note, is there anything I can do to give #3062 the same approval + merge treatment?

@MarijnS95 MarijnS95 deleted the linux-bstr branch November 12, 2020 17:17
@ehsannas
Copy link
Contributor

Yeah I understand. If anyone out there is free() ing the pointer, I think they can switch to SysFreeString easily. I agree that it's better to simulate the Windows behavior more accurately here.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Nov 12, 2020 •
edited
Loading

If anyone out there is free() ing the pointer, I think they can switch to SysFreeString easily

@ehsannas The problem is that SysFreeString and friends do not exist outside of Windows, and their shim implementation details on these platforms are uniquely defined by DirectXCompiler. Implementors against libdxcompiler.so currently have to look into the code and assume the way they interact with these shims remains correct.

Hence my request to export these functions (including CoTaskMemFree and SysString*Len) such that there is no guesswork: call it the same as Windows, and it'll just work, even if (though unlikely) shims are modeled differently in the future.

MarijnS95 added a commit to MarijnS95/DirectXShaderCompiler that referenced this pull request Nov 12, 2020
@MarijnS95
dxcisense: Allocate "TM" classes using IMalloc instead of new
b181980 
When running valgrind over a program using `DxcIntelliSense` (to
validate our own deallocations [1]) a bunch of mismatching `new` with
`free()` show up:

    Mismatched free() / delete / delete []
       at 0x483B9AB: free (vg_replace_malloc.c:538)
       by 0x52B06A8: IMalloc::Free(void*) (WinAdapter.cpp:34)
       by 0x6DC7D19: DxcTranslationUnit::Release() (dxcisenseimpl.h:308)
       by 0x14278E: com_rs::unknown::IUnknown::release (unknown.rs:55)
       [...]
     Address 0x4c3c930 is 0 bytes inside a block of size 40 alloc'd
       at 0x483B07F: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:385)
       by 0x6DC06EB: DxcIndex::ParseTranslationUnit(char const*, char const* const*, int, IDxcUnsavedFile**, unsigned int, DxcTranslationUnitFlags, IDxcTranslationUnit**) (dxcisenseimpl.cpp:1192)
       by 0x13020A: hassle_rs::intellisense::ffi::IDxcIndex::parse_translation_unit (macros.rs:108)
       by 0x119B74: hassle_rs::intellisense::wrapper::DxcIndex::parse_translation_unit (wrapper.rs:101)
       [...]

And so on for the other intellisense classes.  All these classes have
`DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL` which deallocates `this` on the
associated `m_pMalloc` with `free()`:

    The "TM" version keep an IMalloc field that, if not null, indicate
    ownership of 'this' and of any allocations used during release.

Yet are allocated using `new`, resulting in this mismatch.  The solution
is to follow a similar approach as the introduction of `IMalloc` to
`DxcIntelliSense` in d5bb308 by rewriting all classes to take an
`IMalloc *` in the constructor and invoking it either through `::Alloc`
from `DXC_MICROCOM_TM_ALLOC` or `CreateOnMalloc`.

[1]: microsoft#3250 (comment)
@MarijnS95 MarijnS95 mentioned this pull request Nov 12, 2020
Merged
pow2clk pushed a commit that referenced this pull request Nov 17, 2020
@MarijnS95
dxcisense: Allocate "TM" classes using IMalloc instead of new (#3258)
5f835c4 
When running valgrind over a program using `DxcIntelliSense` (to
validate our own deallocations [1]) a bunch of mismatching `new` with
`free()` show up:

    Mismatched free() / delete / delete []
       at 0x483B9AB: free (vg_replace_malloc.c:538)
       by 0x52B06A8: IMalloc::Free(void*) (WinAdapter.cpp:34)
       by 0x6DC7D19: DxcTranslationUnit::Release() (dxcisenseimpl.h:308)
       by 0x14278E: com_rs::unknown::IUnknown::release (unknown.rs:55)
       [...]
     Address 0x4c3c930 is 0 bytes inside a block of size 40 alloc'd
       at 0x483B07F: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:385)
       by 0x6DC06EB: DxcIndex::ParseTranslationUnit(char const*, char const* const*, int, IDxcUnsavedFile**, unsigned int, DxcTranslationUnitFlags, IDxcTranslationUnit**) (dxcisenseimpl.cpp:1192)
       by 0x13020A: hassle_rs::intellisense::ffi::IDxcIndex::parse_translation_unit (macros.rs:108)
       by 0x119B74: hassle_rs::intellisense::wrapper::DxcIndex::parse_translation_unit (wrapper.rs:101)
       [...]

And so on for the other intellisense classes.  All these classes have
`DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL` which deallocates `this` on the
associated `m_pMalloc` with `free()`:

    The "TM" version keep an IMalloc field that, if not null, indicate
    ownership of 'this' and of any allocations used during release.

Yet are allocated using `new`, resulting in this mismatch.  The solution
is to follow a similar approach as the introduction of `IMalloc` to
`DxcIntelliSense` in d5bb308 by rewriting all classes to take an
`IMalloc *` in the constructor and invoking it either through `::Alloc`
from `DXC_MICROCOM_TM_ALLOC` or `CreateOnMalloc`.

[1]: #3250 (comment)
@MarijnS95
Copy link
Contributor Author

@ehsannas I'm afraid I forgot to tag you in the reply above and it seems like you have not received a notification for it. Are there any objections to exporting aforementioned shimmed functions for use on non-Windows platforms?

@ehsannas
Copy link
Contributor

I'm afraid I'm not following what you exactly mean by exporting them. Would you please make a separate PR so that discussions can take place there? Thanks

@MarijnS95 MarijnS95 mentioned this pull request Nov 17, 2020
Open
MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Nov 25, 2020
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit size
1c80cf8 
Since microsoft/DirectXShaderCompiler#3250
@MarijnS95 MarijnS95 mentioned this pull request Nov 25, 2020
Merged
@Jasper-Bekkers Jasper-Bekkers mentioned this pull request Dec 10, 2020
Open
38 tasks
MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Dec 29, 2020
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit size
ca50c59 
Since microsoft/DirectXShaderCompiler#3250
MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Mar 15, 2021
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit size
ed6ea7e 
We [recently] fixed memory leaks when calling into the intellisense part
of DXC and made the decision shortly after merging to [implement
null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows,
which was promptly accepted.

Currently our implementation of `SysFreeString` together with that PR
segfaults because it frees the pointer at offset `+4` of the allocation.
That is corrected together with `SysStringLen` now reading the size of
the string from this prefix instead of looking for a (wrong!) null
character. At least our Windows and Linux implementation `utils.rs` is
the same again.

[recently]: #10
[implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250
MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request May 5, 2021
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit size
81d91d7 
We [recently] fixed memory leaks when calling into the intellisense part
of DXC and made the decision shortly after merging to [implement
null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows,
which was promptly accepted.

Currently our implementation of `SysFreeString` together with that PR
segfaults because it frees the pointer at offset `+4` of the allocation.
That is corrected together with `SysStringLen` now reading the size of
the string from this prefix instead of looking for a (wrong!) null
termination character.  At least our Windows and Linux implementation
`utils.rs` is the same again.

[recently]: #10
[implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250
MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request May 5, 2021
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit size
8edb988 
We [recently] fixed memory leaks when calling into the intellisense part
of DXC and made the decision shortly after merging to [implement
null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows,
which was promptly accepted.

Currently our implementation of `SysFreeString` together with that PR
segfaults because it frees the pointer at offset `+4` of the allocation.
That is corrected together with `SysStringLen` now reading the size of
the string from this prefix instead of looking for a (wrong!) null
termination character.  At least our Windows and Linux implementation
`utils.rs` is the same again.

[recently]: #10
[implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250
Jasper-Bekkers pushed a commit to Traverse-Research/hassle-rs that referenced this pull request May 5, 2021
@MarijnS95
Linux: DXC BSTR now resembles its Windows counterpart with explicit s…
9467024 
…ize (#11)

We [recently] fixed memory leaks when calling into the intellisense part
of DXC and made the decision shortly after merging to [implement
null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows,
which was promptly accepted.

Currently our implementation of `SysFreeString` together with that PR
segfaults because it frees the pointer at offset `+4` of the allocation.
That is corrected together with `SysStringLen` now reading the size of
the string from this prefix instead of looking for a (wrong!) null
termination character.  At least our Windows and Linux implementation
`utils.rs` is the same again.

[recently]: #10
[implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehsannas ehsannas ehsannas approved these changes

Assignees
No one assigned
Labels
linux Linux-specific work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarijnS95 @AppVeyorBot @ehsannas