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.

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

dxcisense: Allocate "TM" classes using IMalloc instead of new #3258

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

MarijnS95
Copy link
Contributor

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.

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)
@AppVeyorBot
Copy link

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.

Thanks for your contribution @MarijnS95

@pow2clk
Copy link
Member

pow2clk commented Nov 17, 2020

I'm very pleased to see this because when I ran valgrind myself some months ago, this was high on the list of errors.

However, I'm going to have to defer to @jeffnn to verify this doesn't break anything that calls these interfaces. Not necessarily a dealbreaker if it does, but we need to proceed more cautiously if they do.

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

We're good to go on our side!

@pow2clk pow2clk merged commit 5f835c4 into microsoft:master Nov 17, 2020
@MarijnS95 MarijnS95 deleted the intellisense-fix-mixed-new-free branch November 17, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants