ci: fix false negative asan checks

[scottwittenburg]

The asan tests have all been failing like this for some unknown amount of time, and it seems that kind of failure didn't trigger ctest to exit non-zero, which caused the check to look like it was passing.

To start with, let's see if we can get the test to actually run all those tests and see if there are any real failures. If there are, we will have to fix them here, before merging this.

[scottwittenburg]

Yikes, a lot of errors detected by asan now, but at least they're showing up properly in cdash.

And quite a few of them look very similar, so maybe not as many problem sources at it might seem at first.

[eisenhauer]

OK, at least one seems to be BP5, perhaps related to support for readRandomAccess, which makes sense as we're jumping through a lot of hoops to keep all that metadata around and without msan we might have missed issues. I can take point on resolving the actual errors for the moment.

[scottwittenburg]

I can take point on resolving the actual errors for the moment.

@eisenhauer What do you think of using this ctest feature to ignore the current failures, so we can merge this PR asap? That way we can prevent new issues from creeping in while we work on fixing the existing failures (in a new PR that adds back the current failing tests).

[eisenhauer]

I can take point on resolving the actual errors for the moment.

@eisenhauer What do you think of using this ctest feature to ignore the current failures, so we can merge this PR asap? That way we can prevent new issues from creeping in while we work on fixing the existing failures (in a new PR that adds back the current failing tests).

Hmm. My first reaction would be to try to kill a few of the easier sanitizer issues and then resort to ignore for those that aren't trivial. But a counter-argument might be that if we fix san bugs in separate PRs, that makes it easier to backport those fixes into release_29. I guess I don't have strong feelings either way...

[vicentebolea]

But a counter-argument might be that if we fix san bugs in separate PRs, that makes it easier to backport those fixes into release_29. I guess I don't have strong feelings either way...

Thats indeed important, we cannot backport API/ABI incompatible changes such as the change of the function signature. I suggest to first add the ignore directive and later we fix this issues one by one, this will make the backporting process much easier.

[eisenhauer]

Thats indeed important, we cannot backport API/ABI incompatible changes such as the change of the function signature. I suggest to first add the ignore directive and later we fix this issues one by one, this will make the backporting process much easier.

Lets follow that approach. I'm also concerned that the overall MinMax fix may be more complex than my changes above, so I'd propose that you guys roll back my commits, add the CTEST_CUSTOM_MEMCHECK_IGNORE spec and merge this PR.

[vicentebolea]

Great work here 👍

[scottwittenburg]

Hmmm, all checks passed, approval from @vicentebolea, what requirements have yet to be met?

[scottwittenburg]

what requirements have yet to be met?

I guess we require branch to be up to date before merging.

[vicentebolea]

Hmmm, all checks passed, approval from @vicentebolea, what requirements have yet to be met?

The option requires PR to be uptodate was enabled. We had some issues in some earlier pull requests that the CI was passing with this automatic merge commit but once it was merged it broke some tests. Maybe the reason was that a new commit got to master right before that was merge and that change was not tested.

[vicentebolea]

I disabled it again since our CI is in a good state again. I enabled it because the CI was not in a good state and I was trying to avoid further issues until the current issues were resolved.