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

Leak fix #48

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pretranslated/14/index.d
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,16 @@ alias CXIndex = void*;
/**
* \brief A single translation unit, which resides in an index.
*/
struct CXTranslationUnitImpl;
struct CXTranslationUnitImpl {
Copy link
Author

Choose a reason for hiding this comment

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

clang from 14 to at least 19 uses same struct

Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need the definition of it though?

Copy link
Author

@denizzzka denizzzka Jun 27, 2024

Choose a reason for hiding this comment

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

To hide store pointer to the TranslationUnit inside of one of the unused fields (CommentToXML), so that this pointer is accessible to any cursor created from CXTranslationUnit

Copy link
Author

Choose a reason for hiding this comment

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

Other ways involve massive Cursor interface changes

Copy link
Owner

Choose a reason for hiding this comment

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

We would change the interface of a type that's from libclang??

Copy link
Author

Choose a reason for hiding this comment

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

Why is that?

As I remember, Cursor becomes mutable and it is leads to changes in almost all functions what use Cursor

Copy link
Contributor

@aminya aminya Jul 15, 2024

Choose a reason for hiding this comment

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

It would be counter productive to mark code with opaque memory leaks as safe.
Regarding the supported version, that can be easily resolved by bumping the major version. There's no point in supporting old libclang libraries.

Copy link
Author

Choose a reason for hiding this comment

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

It would be counter productive to mark code with opaque memory leaks as safe.

This is a reason for another PR, there is no need to clutter this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that it's better to rely on the definition to fix the memory leaks instead of trying to keep the library compatible with older versions of libclang.

Copy link
Author

Choose a reason for hiding this comment

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

instead of trying to keep the library compatible with older versions of libclang.

I don't have such a goal - I'm only worried about the interface changes that affect dpp (it heavy uses libclang)

void* CIdx;
void* TheASTUnit;
void* StringPool;
void* Diagnostics;
void* OverridenCursorsPool;
void* CommentToXML;
uint ParsingOptions;
void* Arguments;
};
alias CXTranslationUnit = CXTranslationUnitImpl*;

/**
Expand Down
11 changes: 10 additions & 1 deletion pretranslated/15/index.d
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,16 @@ alias CXTargetInfo = CXTargetInfoImpl*;
/**
* A single translation unit, which resides in an index.
*/
struct CXTranslationUnitImpl;
struct CXTranslationUnitImpl {
void* CIdx;
void* TheASTUnit;
void* StringPool;
void* Diagnostics;
void* OverridenCursorsPool;
void* CommentToXML;
uint ParsingOptions;
void* Arguments;
};
alias CXTranslationUnit = CXTranslationUnitImpl*;

/**
Expand Down
70 changes: 54 additions & 16 deletions source/clang/package.d
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ TranslationUnit parse(in string fileName,
// faux booleans
const excludeDeclarationsFromPCH = 0;
const displayDiagnostics = 0;
auto index = clang_createIndex(excludeDeclarationsFromPCH, displayDiagnostics);
scope(exit) clang_disposeIndex(index);
CXIndex index = clang_createIndex(excludeDeclarationsFromPCH, displayDiagnostics);
CXUnsavedFile[] unsavedFiles;
const commandLineArgz = commandLineArgs
.map!(a => a.toStringz)
Expand Down Expand Up @@ -79,7 +78,7 @@ TranslationUnit parse(in string fileName,
errorMessages.join("\n")));


return TranslationUnit(cx);
return new TranslationUnit(index, cx);
}

string[] systemPaths() @safe {
Expand Down Expand Up @@ -122,24 +121,34 @@ string[] systemPaths() @safe {
mixin EnumD!("ChildVisitResult", CXChildVisitResult, "CXChildVisit_");
alias CursorVisitor = ChildVisitResult delegate(Cursor cursor, Cursor parent);

struct TranslationUnit {

class TranslationUnit {
private CXIndex index;
CXTranslationUnit cx;
Cursor cursor;

this(CXTranslationUnit cx) @safe nothrow {
this(CXIndex index, CXTranslationUnit cx) @safe nothrow {
this.index = index;
this.cx = cx;
this.cursor = Cursor(clang_getTranslationUnitCursor(cx));

assert(cx.CommentToXML is null);
() @trusted {
cx.CommentToXML = cast(void*) this;
}();
}

// This pair of functions *should* work but crash dpp's tests intead
// A memory leak is a better than a crash, so...
~this() @safe @nogc pure nothrow {
() @trusted {
assert(cx.CommentToXML == cast(void*) this);
}();

// @disable this(this);
cx.CommentToXML = null;
clang_disposeTranslationUnit(cx);
clang_disposeIndex(index);
}

// ~this() @safe @nogc pure nothrow {
// clang_disposeTranslationUnit(cx);
// }
Cursor cursor() @safe nothrow
{
return Cursor(clang_getTranslationUnitCursor(cx));
}

string spelling() @safe pure nothrow const {
return clang_getTranslationUnitSpelling(cx).toString;
Expand Down Expand Up @@ -206,6 +215,7 @@ struct Cursor {

alias Hash = ReturnType!clang_hashCursor;

private TranslationUnit trUnit;
CXCursor cx;
private Cursor[] _children;
Kind kind;
Expand All @@ -224,6 +234,8 @@ struct Cursor {

if(kind == Cursor.Kind.TypedefDecl || kind == Cursor.Kind.TypeAliasDecl)
underlyingType = Type(clang_getTypedefDeclUnderlyingType(cx));

trUnit = fetchTranslationUnit();
}

this(in Kind kind, in string spelling) @safe @nogc pure nothrow {
Expand All @@ -237,6 +249,15 @@ struct Cursor {
this.type = type;
}

this()(ref return Cursor s) @safe @nogc pure nothrow {
cx = s.cx;
kind = s.kind;
type = s.type;
underlyingType = s.underlyingType;

trUnit = fetchTranslationUnit();
}

/// Lazily return the cursor's children
auto children(this This)() @property {
import std.array: appender;
Expand Down Expand Up @@ -428,8 +449,8 @@ struct Cursor {
return Cursor(clang_getSpecializedCursorTemplate(cx));
}

TranslationUnit translationUnit() @safe nothrow const {
return TranslationUnit(clang_Cursor_getTranslationUnit(cx));
TranslationUnit translationUnit() @safe nothrow {
return trUnit;
}

Language translationUnitLanguage() @safe pure nothrow const {
Expand Down Expand Up @@ -618,6 +639,23 @@ struct Cursor {
private SourceRange _sourceRangeCreate() @safe pure nothrow const {
return SourceRange(clang_getCursorExtent(cx));
}

private TranslationUnit fetchTranslationUnit() @safe @nogc pure nothrow const
{
assert(trUnit is null, "must be called once only from ctor");

CXTranslationUnitImpl* tui = clang_Cursor_getTranslationUnit(cx);

// can be null if cursor created by cxcursor::MakeCXCursorInvalid
if(tui is null)
return null;

return () @trusted {
TranslationUnit tu = cast(TranslationUnit) tui.CommentToXML;
assert(tu !is null);
return tu;
}();
}
}


Expand Down
Loading