Skip to content

Fix ASTC Decode_astc_8x8_unorm_array_7 testcase segfault#1176

Merged
MarkCallow merged 6 commits into
KhronosGroup:mainfrom
walcht:fix-test-segfault
Jun 13, 2026
Merged

Fix ASTC Decode_astc_8x8_unorm_array_7 testcase segfault#1176
MarkCallow merged 6 commits into
KhronosGroup:mainfrom
walcht:fix-test-segfault

Conversation

@walcht

@walcht walcht commented May 25, 2026

Copy link
Copy Markdown
Contributor

As partially discussed here: #1173.

I skimmed through some tests and by grepping for "KTX_SUCCESS" all checks almost always use EXPECT_XX which should, probably, be changed to ASSERT_XX (because why proceed to a segfault/other failure when a core Ktx function returned an exit error code?).

walcht and others added 2 commits May 25, 2026 21:58
*Running tests without using git-lfs (e.g.,
`GIT_LFS_SKIP_SMUDGE=1 git clone <repo>`) causes a single segfault test
failure for ASTC Decode_astc_8x8_unorm_array_7 test case. Ideally, all
checks with KTX_SUCCESS should be changed from EXCEPT_XX to ASSERT_XX so
that tests exit early and properly and not via a segfault.

Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
@MarkCallow

Copy link
Copy Markdown
Collaborator

After I merge this, you will no longer be a "first-time contributor" so pushes thereafter to PR #1167 will start a CI run. Is #1167 ready for that?

@MarkCallow

Copy link
Copy Markdown
Collaborator

I skimmed through some tests and by grepping for "KTX_SUCCESS" all checks almost always use EXPECT_XX which should, probably, be changed to ASSERT_XX (because why proceed to a segfault/other failure when a core Ktx function returned an exit error code?).

Only the KTX_SUCCESS checks on functions that will return a nullptr on failure, e.g. ktxTexture?_Create*, need to be changed. For those cases where the following nullptr check outputs a message, the message must be moved to the KTX_SUCCESS check.

Please make these changes, if you have time.

Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
@walcht

walcht commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

So I looked at other KTX_SUCCESS checks, and they all seem to make sense. Some are:

EXPECT_EQ(result, KTX_SUCCESS);
ASSERT_TRUE(texture != NULL) << "ktxTexture?_Create failed: " << ktxErrorString(result);

While others:

ASSERT_TRUE(result == KTX_SUCCESS);
ASSERT_TRUE(texture != NULL) << "ktxTexture_CreateFromMemory failed: " << ktxErrorString(result);

Some others do not assert on texture != nullptr but have reasonable if statements afterwards:

if (texture) { /* other stuff/tests */ }

So I just changed the single check that is causing the issue without touching anything else.

Is #1167 ready for that?

Finally, yes.

@MarkCallow

Copy link
Copy Markdown
Collaborator
ASSERT_TRUE(result == KTX_SUCCESS);
ASSERT_TRUE(texture != NULL) << "ktxTexture_CreateFromMemory failed: " << ktxErrorString(result);

IIUC, in this case the test will stop if result != KTX_SUCCESS which means the error string for result will not be printed. Please change these to EXPECT_EQ.

Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
@MarkCallow

Copy link
Copy Markdown
Collaborator

Please merge the latest main which fixes the cloudflare-related Windows CI failure.

@MarkCallow

Copy link
Copy Markdown
Collaborator

Please merge latest main here and in PR #1167.

@MarkCallow

Copy link
Copy Markdown
Collaborator

I'll come back in an hour and if you've done it, I'll run the builds.

@MarkCallow MarkCallow merged commit 03972f5 into KhronosGroup:main Jun 13, 2026
40 checks passed
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.

2 participants