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 generation of invalid LLVM IR #4506

Merged
merged 7 commits into from
Apr 30, 2024
Merged

Fix generation of invalid LLVM IR #4506

merged 7 commits into from
Apr 30, 2024

Conversation

ArthurPV
Copy link
Contributor

Fix #4475

Before this change, LLVM reported an error during module verification when trying to compile the code referenced in issue #4475:

class Foo
  new create(a: U32) ? =>
    error

actor Main
  new create(env: Env) =>
    try
      let f = Foo(1)?
    end

This bug was caused by the failure to check for the presence of terminator instructions when generating ret instructions. So now, with this change, there's a check for the absence of a terminator instruction each time a ret instruction is generated.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 17, 2024
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Apr 17, 2024
@ponylang-main
Copy link
Contributor

Hi @ArthurPV,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4506.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen SeanTAllen changed the title Fix: Invalid LLVM generated Fix generation of invalid LLVM IR Apr 17, 2024
@SeanTAllen
Copy link
Member

Can you switch to using a runner test?

https://github.com/ponylang/ponyc/tree/main/test/full-program-tests

They don't have lots of gotchas about what is tested.

@SeanTAllen
Copy link
Member

SeanTAllen commented Apr 17, 2024

Ideally both the examples from the issue would have runner tests.

See the regression 623 tests for naming for having more than 1 test. And check out the main.pony for each for the standard sort of comment that should appear in the tests to indicate the original issue being addressed.

@SeanTAllen
Copy link
Member

I've verified that this fixes the issue. I wanted to add an extra layer of verification "just in case" something was wrong with tests that I didn't see. I built ponyc from @ArthurPV's branch. It's looking good in terms of fix.

Thanks @ArthurPV.

@jemc any change requests?

@jemc jemc merged commit b949388 into ponylang:main Apr 30, 2024
22 checks passed
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Apr 30, 2024
github-actions bot pushed a commit that referenced this pull request Apr 30, 2024
github-actions bot pushed a commit that referenced this pull request Apr 30, 2024
@ArthurPV ArthurPV deleted the fix_4475 branch September 27, 2024 23:38
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.

Invalid LLVM generated
4 participants