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

Fix codegen failures on incompatible FFI declarations #2205

Merged
merged 1 commit into from
Sep 1, 2017

Conversation

Praetonus
Copy link
Member

This change fixes a bug where the same FFI function was being declared with incompatible signatures in different packages without being validated, causing various crashes from both ponyc and LLVM.

Closes #1552.

@Praetonus Praetonus added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Aug 31, 2017
@Praetonus Praetonus force-pushed the fix-1552 branch 3 times, most recently from cfdec96 to 48e8229 Compare September 1, 2017 12:45
@Praetonus
Copy link
Member Author

I've commented out a test in BadPony which is now failing because of changes I've made to the unit test runner. That test was testing library mode stuff, which isn't implemented in unit tests yet. I'll add the ability to run library mode tests later and I'll uncomment the test at that moment.

bool is_func = false;
LLVMValueRef func = LLVMGetNamedGlobal(c->module, f_name);

if(func == NULL)
Copy link
Member

@jemc jemc Sep 1, 2017

Choose a reason for hiding this comment

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

Can you add a code comment here that says in what cases the func will be retrieved from LLVMGetNamedFunction instead of LLVMGetNamedGlobal? I see that this case wasn't here before, so I'm curious what Pony types this corresponds to.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the name corresponds to a function, it will always be retrieved from LLVMGetNamedFunction. The LLVMGetNamedGlobal call is here to catch the cases where the name corresponds to an internal global and issue a compilation error for those cases.

I'll add a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, yeah, I had it backwards. Thanks for clearing it up for mw.

This change fixes a bug where the same FFI function was being declared
with incompatible signatures in different packages without being
validated, causing various crashes from both ponyc and LLVM.

Closes ponylang#1552.
Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

Looks great. Feel free to merge when CI passes.

@Praetonus Praetonus merged commit f3b5eda into ponylang:master Sep 1, 2017
@Praetonus Praetonus deleted the fix-1552 branch September 1, 2017 20:36
ponylang-main added a commit that referenced this pull request Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FFI call LLVM type signatures are not checked for conflicts
2 participants