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

Make the verify pass on by default #4036

Merged
merged 3 commits into from
Oct 29, 2023
Merged

Make the verify pass on by default #4036

merged 3 commits into from
Oct 29, 2023

Conversation

SeanTAllen
Copy link
Member

As we slowly move towards Pony 1.0.0, fixing issues in LLVM IR
generation is an important consideration. We've turned on
LLVM IR generation verification as a default in the compiler.

Hopefully this will generate reports for failures that are
currently hidden in the wild.

@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Feb 23, 2022
@SeanTAllen SeanTAllen requested a review from a team February 23, 2022 03:50
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 23, 2022
@SeanTAllen
Copy link
Member Author

Two of our runner tests currently fail verification. We'll need to fix those (and other errors that might be hidden by those failures) before this is merged.

@jemc
Copy link
Member

jemc commented Feb 23, 2022

Suggested change to make verification failure more friendly and make people aware that they can turn it off if they run into an error:

diff --git a/src/libponyc/codegen/genopt.cc b/src/libponyc/codegen/genopt.cc
index 970e43fc..0761075d 100644
--- a/src/libponyc/codegen/genopt.cc
+++ b/src/libponyc/codegen/genopt.cc
@@ -1446,6 +1446,8 @@ bool genopt(compile_t* c, bool pony_specific)
     if(LLVMVerifyModule(c->module, LLVMPrintMessageAction, &msg) != 0)
     {
       errorf(errors, NULL, "Module verification failed: %s", msg);
+      errorf_continue(errors, NULL,
+        "Please file an issue ticket. Use --noverify to bypass this error.");
       LLVMDisposeMessage(msg);
       return false;
     }

@SeanTAllen
Copy link
Member Author

@jemc added.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Feb 23, 2022
@SeanTAllen
Copy link
Member Author

This shouldnt be merged until we fix all the associated issues that cause CI to fail.

@jemc jemc removed the discuss during sync Should be discussed during an upcoming sync label Mar 8, 2022
As we slowly move towards Pony 1.0.0, fixing issues in LLVM IR
generation is an important consideration. We've turned on
LLVM IR generation verification as a default in the compiler.

Hopefully this will generate reports for failures that are
currently hidden in the wild.
@SeanTAllen
Copy link
Member Author

I've rebased this against main. I assume it will still fail CI, but we can at least see where we are at with this.

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

jemc commented Oct 10, 2023

The only test failing (at least on the Linux build that I checked) is the full program test named with-return - it's getting the IR verification error that there is a "Terminator found in the middle of a basic block!".

We need to fix something about the interaction of an explicit return inside a with block. Should be fairly simple I think?

@SeanTAllen
Copy link
Member Author

We need to fix something about the interaction of an explicit return inside a with block. Should be fairly simple I think?

Well, maybe? the issue is, we wouldn't want both returns if they end up together, but a return from a with is perfectly fine the issue is that it is the last statement and our "no explicit return except early doesn't cover this"

@SeanTAllen
Copy link
Member Author

We are going to remove the with-return test for now as it is flawed. See #4464.

@SeanTAllen
Copy link
Member Author

This is looking good, but I don't want to merge yet. I want to get a release that solves #4454

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Oct 29, 2023
@SeanTAllen SeanTAllen merged commit 219a640 into main Oct 29, 2023
22 checks passed
@SeanTAllen SeanTAllen deleted the always-verify branch October 29, 2023 02:29
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Oct 29, 2023
github-actions bot pushed a commit that referenced this pull request Oct 29, 2023
github-actions bot pushed a commit that referenced this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants