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

[CI] Test against LLVM 13 #11343

Merged

Conversation

straight-shoota
Copy link
Member

  • Adds a test_llvm job which test against specific LLVM versions. Adding only LLVM 13 for now. LLVM 10 is technically also possible, but it breaks. Needs more investigation. Older LLVM releases are not easily available as binary tarballs.
  • Updates windows-job to use LLVM 13. We don't need to test older releases for now.
  • Removes the workaround for Codegen error on win32 #11047 when building with LLVM 13

Follow-up on #11302

Resolves #11047

Adding only LLVM 13 for now. LLVM 10 is technically also possible, but
it breaks. Needs more investigation. Older LLVM releases are not
easily available as binary tarballs.
@straight-shoota
Copy link
Member Author

We could also consider this action for installing LLVM. https://github.com/marketplace/actions/install-llvm-and-clang

It supports older releases as well, and works cross-platform. But it's an additional dependency 🤷
For future release, we can probably expect to be able to just grab the binary distribution package from LLVM directly.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Regarding LLVM 10, I'd be careful of not re-running tests that are checked by other means. I mean, IIUC we're already testing that version in other runs, aren't we?

@straight-shoota
Copy link
Member Author

Yes, but the main tests are not fixed to LLVM. We should probably try to upgrade as soon as LLVM 13 is available.
At that point we might forget about this. I would prefer to have explicit specs for LLVM 10.
But that's a story for a follow-up.

@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 2, 2021
@straight-shoota
Copy link
Member Author

straight-shoota commented Dec 6, 2021

Somehow the LLVM 13 specs have started failing with weird errors in compiler_spec.

While trying to reproduce locally, I already had issues with getting the entire thing to compiler. For some reason, I needed GCC 11. But apparently, that's not necessary on CI (the build docker image ships GCC 9.3). Maybe it's just that GCC 10 causes trouble (which I'm using locally).

Anyways, I got the second error on my machine as well. It's in codegens abstract struct with module include (#11385) (added in
#11390). Maybe the first one was just a glitch 🤷

The location of this failure explains why this failure hadn't come up before and should remind us to focus more on getting CI specs in order. This PR was sitting around for two months and in the mean time we merged a change that breaks CI.

/cc @maxfierke You contributed both LLVM 13 support as well as the faulty spec, do you have any leads what this might be about?

@maxfierke
Copy link
Contributor

/cc @maxfierke You contributed both LLVM 13 support as well as the faulty spec, do you have any leads what this might be about?

Do you know what test specifically this is happening on?

Anyways, I got the second error on my machine as well. It's in codegens abstract struct with module include (#11385) (added in #11390). Maybe the first one was just a glitch 🤷

I believe this spec got reverted here: #11470. The spec example was pulled from #11384 but ultimately I was trying to fix #11385 which was presenting similarly but didn't have a reproduction. I would suspect there's still a bug there for that sort of implicit recursive struct creation, but it sounds like maybe not something that should ultimately be allowed?

The LLVM 13 changes to debug types were really about making sure the right LLVM context for the right LLVM module was used with the debug types being generated.

@straight-shoota
Copy link
Member Author

Do you know what test specifically this is happening on?

I got this on a local run as well now, in Semantic: array types array literal of union.

I'm really confused now. You're right, that spec has been removed #11470. And that commit has been merged into this branch. So the failure in CI can be disregarded, it was before the removal. Still, it also showed up in my local spec run. Apparently, I messed that up somehow. But I can't see how. However, I can't recall that particular instance anymore because the console buffer runs full quickly with the verbose output 😆 It's a mystery.

So the free(): invalid pointer failure seems to be the actual issue currently.

@straight-shoota
Copy link
Member Author

The error appears in different locations, but all I've seen are in array_spec.cr. It does not reproduce when running array_spec.cr in isolation, though. I suppose it happens when the GC does a cleanup round.

@straight-shoota straight-shoota removed this from the 1.3.0 milestone Dec 7, 2021
@beta-ziliani
Copy link
Member

On the tip of master I got it passing locally on my Mac. I wonder if this is a 2nd gen compiler thing (my compiler was compiled by the previous one also compiled with LLVM 13).

@straight-shoota
Copy link
Member Author

I tried with first and second generation compiler locally in the crystallang/crystal:1.2.2-build image: make clean deps crystal && make compiler_spec; touch src/compiler/crystal.cr && make crystal && make compiler_spec
Both failed with the same invalid pointer error (although in different locations, but we already knew that).

@beta-ziliani
Copy link
Member

This is going to be fun

@beta-ziliani
Copy link
Member

In macOS, I can confirm that compiling the compiler with 1.2.2 (LLVM 11.1) and 1.3-dev (LLVM 13) produces a compiler that passes the specs successfully.

@straight-shoota straight-shoota added this to the 1.4.0 milestone Jan 5, 2022
@straight-shoota straight-shoota merged commit c5d7695 into crystal-lang:master Jan 21, 2022
@straight-shoota straight-shoota deleted the feature/ci-llvm13 branch January 21, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codegen error on win32
3 participants