-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add andersen region-aware CI job [AndersenRegionAware] #526
base: master
Are you sure you want to change the base?
Conversation
This is the same single source test that also failed for Steensgaard region-aware. Moreover, we did not have any problem in the run when we added the target to the llvm-test-suite. Ultimately, it means we should invest in some bigger/better/heavier guns for debugging. The problem arises due to the alias analysis in combination with the region-aware encoder relaxing the sequentialization order, and depending on which sequence the back-end chooses, it can happen that we get a valid one according to program semantics, or maybe a faulty one, because of bugs in the alias analysis/provisioning/encoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI change itself looks good. I definitely agree that some hardening of tests is needed to avoid flakey CI jobs
@haved I believe that has nothing to do with flaky CI jobs. My suspicion is that this is a bug in the alias analysis and/or the encoding. The region-aware encoding relaxes the sequential order of the operations with side-effects. This means that the jlm back-end could emit a wrong order of operations if there is a bug in any of those passes. This would not happen with the agnostic encoding (at least not without further transformations such as invariant value redirection + dead node elimination). |
Let's not merge this before I have not found out what the root cause is. |
@phate, I agree with your description of the problem. Calling it "flakey CI" was not a good description on my part. In general there seems to be multiple places in the compiler where non-determinism is introduced due to iterating over sets and maps keyed by addresses. As long as there are no bugs this is not an issue, but when there are, it can make it more difficult to reproduce. This non-determinism combined with the current bug you describe is why I referred to the potential CI job as flaky. Even if the CI job fails consistently, it is no fun when you run a failing job locally and no issues occur. To make bugs more reproducible, I see a few options, but there are probably many more:
|
@haved Right, I see your point with flaky CI now. Yes, having the compiler behave differently/non-deterministically is not great for reproducibility. Regarding your mentioned options:
@caleridas might have something to say here as well. |
Long-term, any indeterminism should be rooted out from the compiler, and not just in debug builds. (I confess to being guilty to introducing indeterministic behavior to begin with, and it was a bad idea). iterating HashSet/HashMap should mostly be "forbidden" unless the iteration has no effect on output (e.g. just deleting from the map, or copying things somewhere else), or the result is brought into a deterministic order afterwards. The strategy that I have observed to work best in various projects to get rid of this problem is to "intentionally" introduce into hash maps and add validation tests that fail if multiple runs fail to produce bit-identical output. |
Aiming to remove non-determinism would be very nice, and making it a part of automatic testing sounds like a good plan long-term. I agree that it is a better solution to not rely on any ordering of HashSets keyed with addresses. I have created discussion #529 to not discuss all of this in this PR. Regarding non-determinism from malloc, I have for some reason gotten very consistent results even though ASLR is enabled. I have had logic bugs that caused every execution to give the wrong result when executed normally, but never give the wrong result when executed in gdb. This was not a memory bug, so the only possible difference was iteration order in sets of addresses. |
No description provided.