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.

Sign up for GitHub

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

Rework Dynamic Analysis and sanitize testing #4681

Merged
merged 4 commits into from
Aug 5, 2024
+139 −163
Merged

Rework Dynamic Analysis and sanitize testing #4681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b75c31
Ignore predetermined failing test and check pointer before use
byrnHDF Jul 30, 2024
c5c17cd
Rework Analysis process
byrnHDF Jul 31, 2024
5a87ec7
Remove unused file
byrnHDF Jul 31, 2024
ee0d26e
Merge branch 'develop' of https://github.com/HDFGroup/hdf5.git into d…
byrnHDF Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions tools/lib/h5diff_array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1309,10 +1309,11 @@ all_zero(const void *_mem, size_t size)
{
const unsigned char *mem = (const unsigned char *)_mem;

while (size-- > 0)
if (mem[size])
return false;

if (mem != NULL) {
while (size-- > 0)
if (mem[size])
return false;
}
return true;
}

Expand Down
9 changes: 5 additions & 4 deletions tools/lib/h5tools_str.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,10 +1587,11 @@ h5tools_str_is_zero(const void *_mem, size_t size)
{
const unsigned char *mem = (const unsigned char *)_mem;

while (size-- > 0)
if (mem[size])
return false;

if (mem != NULL) {
while (size-- > 0)
if (mem[size])
return false;
}
return true;
}

Expand Down
22 changes: 14 additions & 8 deletions tools/test/h5ls/CMakeTests.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,22 +320,28 @@
# rather than "IEEE 16-bit little-endian float".
if (H5_WORDS_BIGENDIAN)
ADD_H5_TEST (tfloat16_be 0 -w80 -v tfloat16_be.h5)
ADD_H5_TEST (tfloat16_be_nosupport 0 -w80 -v tfloat16_be.h5)
set_tests_properties (H5LS-tfloat16_be_nosupport PROPERTIES WILL_FAIL "true")
if (NOT HDF5_USING_ANALYSIS_TOOL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to be checking whether an analysis tool is used when tests that are expected to fail are being tested? Surely there's a better way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trouble with these tests are that the sanitzer will avoid the runtest.cmake macro and therefore the test will not fail. I could execute the test and skip the property setting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it about the sanitizers that causes them to not run through the normal testing macros/framework? Do they do more than just add in the -fsanitize=address flag for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am investigating that now - it might just be valgrind that has the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the sanitizers do not have a problem with the macros - so will convert the checks to MEMCHECKER only.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for a specific analysis tool doesn't seem much better either. I don't necessarily want to suggest adding another test macro for this purpose, but it's not great that anybody adding new tests needs to know that tests which are expected to fail have to be wrapped in if (NOT HDF5_USING_ANALYSIS_TOOL) or similar. It should be simpler than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked this whole analysis as sanitisers do not have the problems that valgrind has.
In fact, I think the sanitizers negate the need to support valgrind limitations and even the MEMCHECKER checks should just be eliminated.

ADD_H5_TEST (tfloat16_be_nosupport 0 -w80 -v tfloat16_be.h5)
set_tests_properties (H5LS-tfloat16_be_nosupport PROPERTIES WILL_FAIL "true")
endif ()
else ()
ADD_H5_TEST (tfloat16 0 -w80 -v tfloat16.h5)
ADD_H5_TEST (tfloat16_nosupport 0 -w80 -v tfloat16.h5)
set_tests_properties (H5LS-tfloat16_nosupport PROPERTIES WILL_FAIL "true")
if (NOT HDF5_USING_ANALYSIS_TOOL)
ADD_H5_TEST (tfloat16_nosupport 0 -w80 -v tfloat16.h5)
set_tests_properties (H5LS-tfloat16_nosupport PROPERTIES WILL_FAIL "true")
endif ()
endif ()
else ()
# If support is NOT available for _Float16 type, the first two tests
# will fail as the types will be printed out as
# "IEEE 16-bit little-endian float" and "IEEE 16-bit big-endian float"
# rather than "native _Float16"
ADD_H5_TEST (tfloat16 0 -w80 -v tfloat16.h5)
set_tests_properties (H5LS-tfloat16 PROPERTIES WILL_FAIL "true")
ADD_H5_TEST (tfloat16_be 0 -w80 -v tfloat16_be.h5)
set_tests_properties (H5LS-tfloat16_be PROPERTIES WILL_FAIL "true")
if (NOT HDF5_USING_ANALYSIS_TOOL)
ADD_H5_TEST (tfloat16 0 -w80 -v tfloat16.h5)
set_tests_properties (H5LS-tfloat16 PROPERTIES WILL_FAIL "true")
ADD_H5_TEST (tfloat16_be 0 -w80 -v tfloat16_be.h5)
set_tests_properties (H5LS-tfloat16_be PROPERTIES WILL_FAIL "true")
endif ()
ADD_H5_TEST (tfloat16_nosupport 0 -w80 -v tfloat16.h5)
ADD_H5_TEST (tfloat16_be_nosupport 0 -w80 -v tfloat16_be.h5)
endif ()
Expand Down
Loading