search

ponylang / ponyc

Pony is an open-source, actor-model, capabilities-secure, high performance programming language
http://www.ponylang.io
BSD 2-Clause "Simplified" License
5.73k stars 415 forks source link

Fix generation of invalid LLVM IR #4506

Closed ArthurPV closed 6 months ago

ArthurPV commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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?