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

Draft: add windows test runner #133

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

gruenich
Copy link
Contributor

Open issues:

  1. Compiling the SuperLU library fails without an error message.
  2. The optional dependency WinGetOps should be added to the runner to run the tests

Until #131 is merged, this based on top of this branch.

@gruenich gruenich force-pushed the feature/add-windows-test-runner branch 20 times, most recently from af10773 to 585c0dc Compare November 26, 2023 08:47
@gruenich
Copy link
Contributor Author

This branch would add two Windows runners, one with WinGetOpt and one without. The tests are executed for the former. For d_tests are segfaulting.

As pointed out in #108, reverting cf93 fixes the issue.
@xiaoyeli I suggest to revert all changed made as part of #117 as part of this pull request. We have to reach out to @evan-dsa to re-create his changes in a way that they a) do not segfault SuperLU's test suite and b) cover not only double but also the other three types s, c, and z. Do you agree? Should I update this pull request to revert #117 completely?

@gruenich gruenich force-pushed the feature/add-windows-test-runner branch from 8e0a117 to 86d443e Compare November 26, 2023 17:50
@xiaoyeli
Copy link
Owner

All 4 versions should be correct now. Do you still see segfaults?

@gruenich
Copy link
Contributor Author

Thank you for the promt reply. I rebased this branch to include your changes and dropped the reversal of the problematic patch from pull request 117. Unfortunately, we get even more segfaults, not only for double but also for the other types.

@xiaoyeli
Copy link
Owner

The tests are fine with Linux and MacOS. I have no experience with Windows, so I don't know what goes wrong with Windows.

@wo80
Copy link
Contributor

wo80 commented Nov 26, 2023

The tests are fine with Linux and MacOS. I have no experience with Windows, so I don't know what goes wrong with Windows.

Well I hope you are not suggesting to ignore the error.


To tackle the problem, I'd suggest to focus on the test code first, then maybe the SuperLU code.

dtest crashes for imat=7 in dgst01 line 101:

superlu/TESTING/dgst01.c

Lines 94 to 101 in f530ef3

for (k = 0; k < n; ++k) {
/* The U part outside the rectangular supernode */
for (i = U_NZ_START(k); i < U_NZ_START(k+1); ++i) {
urow = U_SUB(i);
utemp = Uval[i];
superno = Lstore->col_to_sup[urow];
fsupc = L_FST_SUPC(superno);

For imat=7 the 9x9 test matrix has all columns > 5 set to zero. At loop k=5 in dgst01 the value for urow gets -1 at some point, which produces a garbage offset for superno. I also checked this on Debian/GCC and it's the same.

One might be lucky and the memory at superno = Lstore->col_to_sup[urow] for urow=-1 is some reasonable number, but I have to express my full support for MSVC producing an access violation exception here!

@wo80
Copy link
Contributor

wo80 commented Nov 27, 2023

Valgrind also catches this correctly:

Invalid read of size 4
   at 0x10C19D: dgst01
   by 0x10B4CA: main
 Address 0x51005dc is 4 bytes before a block of size 40 alloc'd
   at 0x48407B4: malloc (vg_replace_malloc.c:381)
   by 0x117D9F: superlu_malloc
   by 0x117FA3: int32Malloc
   by 0x12588F: dLUMemInit
   by 0x11D830: dgstrf
   by 0x118D35: dgssv
   by 0x10B422: main

...

For lists of detected and suppressed errors, rerun with: -s
ERROR SUMMARY: 40 errors from 12 contexts (suppressed: 0 from 0)

That 4 bytes before a block ... sounds a lot like accessing an integer array at index -1.

@gruenich Maybe add valgrind to the Ubuntu workflow?

BTW, the test command line was ./d_test -t "LA" -n 9 -s 2 -l 10000000, both for valgrind and the debugger in the previous post.

Hopefully the will be resolved later. I don't want to block
related improvements and fixes.
@gruenich gruenich force-pushed the feature/add-windows-test-runner branch from 86d443e to 0559d56 Compare December 1, 2023 21:41
@gruenich
Copy link
Contributor Author

gruenich commented Dec 1, 2023

Moving the segfault issue out of the merge request. I opened #134 for this. I hoped we can fix this here, but I don't want to block other improvements in this merge requests.

I propose to merge this.

@xiaoyeli xiaoyeli merged commit 76b2c9a into xiaoyeli:master Dec 4, 2023
4 checks passed
@gruenich gruenich deleted the feature/add-windows-test-runner branch January 8, 2024 06:59
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