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

Validate more on assertion builds, and abort on errors #52830

Merged
merged 5 commits into from
Jan 14, 2024

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jan 9, 2024

This PR add a jl_is_assertsbuild function (cfr. jl_is_debugbuild) for checking if this is an assertions build.
It's used in two places:

  1. to enable IR validation, which currently (rather weirdly) depends on -g2
  2. to make internal compiler errors fatal

Both these changes are intended to help PkgEval in detecting more issues (invalid Julia IR) and make for more robust detection (a fatal signal instead of having to grep logs for Internal error, which leads to false positives).

RFC because people actually working on the compiler might have opinions about the process not aborting if invalid IR is encountered.

cc @gbaraldi

@gbaraldi
Copy link
Member

gbaraldi commented Jan 9, 2024

There is also

if JLOptions().debug_level == 2
for i = (oldidx + 1):last(sv.cfg.blocks[block].stmts)
@assert i in sv.unreachable
end
end

base/compiler/validation.jl Outdated Show resolved Hide resolved
@maleadt maleadt changed the title RFC: Add and use function to check for assertions build RFC: Validate more on assertion builds, and abort on errors Jan 9, 2024
@maleadt
Copy link
Member Author

maleadt commented Jan 10, 2024

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@maleadt
Copy link
Member Author

maleadt commented Jan 11, 2024

Looking at that report, running with additional verification/validation makes PkgEval on an assertions build 2.3% slower. That seems worth it, given the 12 IR corruptions uncovered here. I'll work on (filing issues to) fix those, and hopefully we can merge this afterwards.

@gbaraldi
Copy link
Member

This found a lot more than I had expected, so I guess it's worthwhile. It's interesting that LLVM doesn't complain about these, because a lot of them seem to be malformed Phi nodes

@maleadt maleadt marked this pull request as draft January 11, 2024 13:14
@maleadt maleadt changed the title RFC: Validate more on assertion builds, and abort on errors Validate more on assertion builds, and abort on errors Jan 11, 2024
@maleadt
Copy link
Member Author

maleadt commented Jan 13, 2024

Keno fixed all of the issues I filed, so let's test again:

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@maleadt maleadt marked this pull request as ready for review January 14, 2024 11:24
@maleadt
Copy link
Member Author

maleadt commented Jan 14, 2024

CI failures unrelated (this only affects asserts builds), and PkgEval shows that this doesn't introduce additional failures.

@maleadt maleadt merged commit 77652fd into master Jan 14, 2024
7 of 9 checks passed
@maleadt maleadt deleted the tb/load_assertions branch January 14, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants