Skip to content

fix malloc/mem.xfree inconsistency in isSameFuncLiteral #23558

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

Merged
thewilsonator merged 1 commit into from
Aug 10, 2026
+14 −15
Merged

fix malloc/mem.xfree inconsistency in isSameFuncLiteral #23558

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 14 additions & 15 deletions compiler/src/dmd/lambdacomp.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,17 @@ private enum ExpType
bool isSameFuncLiteral(FuncLiteralDeclaration l1, FuncLiteralDeclaration l2, Scope* sc)
{
bool result;
if (auto ser1 = getSerialization(l1, sc))
OutBuffer buf1;
if (getSerialization(l1, sc, buf1))
{
OutBuffer buf2;
//printf("l1 serialization: %.*s\n", cast(int)ser1.length, &ser1[0]);
if (auto ser2 = getSerialization(l2, sc))
if (getSerialization(l2, sc, buf2))
{
//printf("l2 serialization: %.*s\n", cast(int)ser2.length, &ser2[0]);
if (ser1 == ser2)
if (buf1.peekSlice() == buf2.peekSlice())
result = true;
mem.xfree(cast(void*)ser2.ptr);
}
mem.xfree(cast(void*)ser1.ptr);
}
return result;
}
Expand All @@ -95,19 +95,17 @@ bool isSameFuncLiteral(FuncLiteralDeclaration l1, FuncLiteralDeclaration l2, Sco
* Params:
* fld = the starting AST node for the lambda function
* sc = the scope in which the lambda function is located
* buf = serialization of `fld` is written to this buffer
*
* Returns:
* The serialization of `fld` allocated with mem.
* true if serialization was successful and was added to buf
*/
private string getSerialization(FuncLiteralDeclaration fld, Scope* sc)
private bool getSerialization(FuncLiteralDeclaration fld, Scope* sc, ref OutBuffer buf)

@kinke kinke Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I suggest changing buf from ref to out, so that the return value is well-defined - it could wrongly return true if the buffer wasn't initially empty, or clear out any existing content in that buffer if the serialization fails. And ideally the Returns: docs would be updated too.

@rainers rainers Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of out parameters because they overwrite struct instances without checking whether they contain anything to cleanup, e.g. passing an OutBuffer with allocated data would cause a leak.

So I kept the ref but only return true if anything was appended. Updated the documentation, too.

@kinke kinke Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see a bunch of buf.length == 0 and buf.setsize(0) in the visitor, so this is clearly not suited for a non-empty buffer. Edit: So an assertion is IMO enough if you wanna keep the ref.

@rainers rainers Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, I was probably assuming "serialization" was just appending. All content is cleared on error AFAICT, though that seems rather brittle (some writes are unconditonal, e.g. the closing ')' of a call).
I changed it to reset the buffer initially.

{
scope serVisitor = new SerializeVisitor(fld.parent._scope);
buf.setsize(0);
scope serVisitor = new SerializeVisitor(fld.parent._scope, &buf);
fld.accept(serVisitor);
const len = serVisitor.buf.length;
if (len == 0)
return null;

return cast(string)serVisitor.buf.extractSlice();
return buf.length != 0;
}

private extern (C++) class SerializeVisitor : SemanticTimeTransitiveVisitor
Expand All @@ -119,12 +117,13 @@ private:
Dsymbol d;

public:
OutBuffer buf;
OutBuffer* buf;
alias visit = SemanticTimeTransitiveVisitor.visit;

this(Scope* sc) scope
this(Scope* sc, OutBuffer* buf) scope
{
this.sc = sc;
this.buf = buf;
}

/**
Expand Down
Loading