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

Add option to use pool implemenation which relies on memalign #4357

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

dipinhora
Copy link
Contributor

This commit add an option to build pony with the pool implementation based on posix_memalign instead of the default pool implementation. This, combined with address/undefined behavior/etc sanitizers, allows for easier debugging of memory issues within the compiler and runtime. This commit also includes two small bugfixes in the compiler that were discovered as part of this:

  • lexer.c fix in normalise_string to prevent issues when lexer->buflen == 0
  • names.c fix in names_typeparam to prevent access to an AST node that was just replaced

and a fix to the Makefile for running tests with use= flags and a fix to make sure the test runner gets rebuilt whenever ponyc gets rebuilt

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jul 5, 2023
@SeanTAllen
Copy link
Member

Is there a particular memory issue etc that you are planning on looking into when this goes in?

Comment on lines 202 to 203
ast = *astp;

Copy link
Member

Choose a reason for hiding this comment

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

is there a known bug from this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not that i'm aware of.. just something that the memory sanitizer flagged (iirc)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4360 created for this

@@ -607,7 +607,7 @@ static void normalise_string(lexer_t* lexer)

// Trim trailing empty line
size_t trim = 0;
for (size_t i = (lexer->buflen - 1); i>0; i--)
for (ssize_t i = (lexer->buflen - 1); i>0; i--)
Copy link
Member

Choose a reason for hiding this comment

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

is there a known bug from this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not that i'm aware of.. just something that the undefined behavior sanitizer flagged (iirc)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4359 created for this

@SeanTAllen
Copy link
Member

Are the two small bugfixes required for this PR? They appear "unrelated-ish" and should probably going as 1 or 2 separate PRs.

@SeanTAllen
Copy link
Member

Is "fix to make sure the test runner gets rebuilt whenever ponyc gets rebuilt" a 3rd separate bug fix that should also be it's own PR?

@dipinhora
Copy link
Contributor Author

Is there a particular memory issue etc that you are planning on looking into when this goes in?

not specifically.. i added this because i was running into some weird crashes on something i'm trying out and i was trying to rule out potential causes and wanted to rely on the memory sanitizer as sanity check..

@dipinhora
Copy link
Contributor Author

Are the two small bugfixes required for this PR? They appear "unrelated-ish" and should probably going as 1 or 2 separate PRs.

Not required.. i'll bump them into their own PRs..

@dipinhora
Copy link
Contributor Author

Is "fix to make sure the test runner gets rebuilt whenever ponyc gets rebuilt" a 3rd separate bug fix that should also be it's own PR?

Yep, it can be it's own PR.. i'll slim this down to only what's needed for the new functionality and break everything else out into separate PRs..

@SeanTAllen
Copy link
Member

Is "fix to make sure the test runner gets rebuilt whenever ponyc gets rebuilt" a 3rd separate bug fix that should also be it's own PR?

Yep, it can be it's own PR.. i'll slim this down to only what's needed for the new functionality and break everything else out into separate PRs..

Awesome

@SeanTAllen
Copy link
Member

@dipinhora can you rebase this against main?

@dipinhora
Copy link
Contributor Author

@dipinhora can you rebase this against main?

done

@SeanTAllen
Copy link
Member

@jemc any objections to this PR?

@SeanTAllen SeanTAllen requested review from jemc and a team July 16, 2023 12:24
This commit add an option to build pony with the pool implementation
based on posix_memalign instead of the default pool implementation.
This, combined with address/undefined behavior/etc sanitizers, allows
for easier debugging of memory issues within the compiler and runtime.
This commit also includes two small bugfixes in the compiler that
were discovered as part of this:

* lexer.c fix in `normalise_string` to prevent issues when
  `lexer->buflen` == 0
* names.c fix in `names_typeparam` to prevent access to an AST node
  that was just replaced

and a fix to the `Makefile` for running tests with `use=` flags
and a fix to make sure the test runner gets rebuilt whenever ponyc
gets rebuilt
@dipinhora
Copy link
Contributor Author

rebased onto main again to ensure proper directory is used when multiple use= flags are enabled.

@SeanTAllen SeanTAllen merged commit 649786d into ponylang:main Jul 18, 2023
17 checks passed
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jul 18, 2023
@dipinhora
Copy link
Contributor Author

@SeanTAllen i made a mistake and my last rebase to this PR undid all the changes that were split off into their own PRs and now main doesn't have any of them any longer:

how do you want to proceed? should i create 5 new PRs to re-apply them or should i create a single one with all the changes combined?

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jul 21, 2023
@SeanTAllen
Copy link
Member

I'll roll this one back later today then redo just this one.

@dipinhora
Copy link
Contributor Author

@SeanTAllen #4367 has been created to do the pool_memalign functionality again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss during sync Should be discussed during an upcoming sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants