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

Fix some warnings in developer builds #3247

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Fix some warnings in developer builds #3247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25a8d82
Fix some warnings in developer builds
jhendersonHDF Jul 14, 2023
713d83e
Switch approach to Winline flag
jhendersonHDF Jul 17, 2023
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
13 changes: 13 additions & 0 deletions config/cmake/HDFCompilerFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,19 @@ else ()
endif ()
endif ()

# Turn off -Winline warning for Developer builds as this flag
# appears to conflict specifically with the -Og optimization
# flag and will produce warnings about functions not being
# considered for inlining under at least GNU compilers
if (CMAKE_C_COMPILER_ID STREQUAL "GNU")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just take it out of the gnu/developer-general file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It still seems like a good warning to have for -O1, -O2, -O3 builds where HDF5_ENABLE_DEV_WARNINGS is on. It might still be able to inform about why a function wasn't inlined and then someone can go inspect. Developer builds have HDF5_ENABLE_DEV_WARNINGS on by default, but if someone is building in RelWithDebInfo or similar with HDF5_ENABLE_DEV_WARNINGS on, then it makes sense to have -Winline there. It's just currently not compatible with what we probably think of as developer builds (developer or debug builds) since we add -Og in both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Secondly, it should probably be in the elseif block of line 187, which is where it would get added. Maybe take it out the developer-general file and only add it back in if supported? All in that elseif block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving into the elseif makes sense. As for taking the flag out and re-adding it later - that sort of seems to me like it defeats the purpose of having the warning flag files in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on whether it is a usually used or a usually removed choice. Which one is more common?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose that depends on how many people usually turn on HDF5_ENABLE_DEV_WARNINGS

if (${HDF_CFG_NAME} MATCHES "Debug" OR ${HDF_CFG_NAME} MATCHES "Developer")
list (FIND H5_CFLAGS "-Winline" h5_have_winline_flag)
if (NOT h5_have_winline_flag EQUAL -1)
list (REMOVE_ITEM H5_CFLAGS "-Winline")
endif ()
endif ()
endif ()

if (CMAKE_C_COMPILER_ID STREQUAL "GNU")
# Technically, variable-length arrays are part of the C99 standard, but
# we should approach them a bit cautiously... Only needed for gcc 4.X
Expand Down
310 changes: 157 additions & 153 deletions src/H5.c
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The changes in this file fix Wjump-misses-init warnings by moving the initialization and termination sections into a new scope block. The malloc attribute also gets added to H5allocate_memory

Original file line number Diff line number Diff line change
Expand Up @@ -243,33 +243,35 @@ H5_init_library(void)
* The dataspace interface needs to be initialized so that future IDs for
* dataspaces work.
*/
/* clang-format off */
struct {
herr_t (*func)(void);
const char *descr;
} initializer[] = {
{H5E_init, "error"}
, {H5VL_init_phase1, "VOL"}
, {H5SL_init, "skip lists"}
, {H5FD_init, "VFD"}
, {H5_default_vfd_init, "default VFD"}
, {H5P_init_phase1, "property list"}
, {H5AC_init, "metadata caching"}
, {H5L_init, "link"}
, {H5S_init, "dataspace"}
, {H5PL_init, "plugins"}
/* Finish initializing interfaces that depend on the interfaces above */
, {H5P_init_phase2, "property list"}
, {H5VL_init_phase2, "VOL"}
};

for (i = 0; i < NELMTS(initializer); i++) {
if (initializer[i].func() < 0) {
HGOTO_ERROR(H5E_FUNC, H5E_CANTINIT, FAIL,
"unable to initialize %s interface", initializer[i].descr)
{
/* clang-format off */
struct {
herr_t (*func)(void);
const char *descr;
} initializer[] = {
{H5E_init, "error"}
, {H5VL_init_phase1, "VOL"}
, {H5SL_init, "skip lists"}
, {H5FD_init, "VFD"}
, {H5_default_vfd_init, "default VFD"}
, {H5P_init_phase1, "property list"}
, {H5AC_init, "metadata caching"}
, {H5L_init, "link"}
, {H5S_init, "dataspace"}
, {H5PL_init, "plugins"}
/* Finish initializing interfaces that depend on the interfaces above */
, {H5P_init_phase2, "property list"}
, {H5VL_init_phase2, "VOL"}
};

for (i = 0; i < NELMTS(initializer); i++) {
if (initializer[i].func() < 0) {
HGOTO_ERROR(H5E_FUNC, H5E_CANTINIT, FAIL,
"unable to initialize %s interface", initializer[i].descr)
}
}
/* clang-format on */
}
/* clang-format on */

/* Debugging? */
H5__debug_mask("-all");
Expand Down Expand Up @@ -349,139 +351,141 @@ H5_term_library(void)
* way that would necessitate some cleanup work in the other interface.
*/

#define TERMINATOR(module, wait) { \
.func = H5##module##_term_package \
, .name = #module \
, .completed = false \
, .await_prior = wait \
}
{
#define TERMINATOR(module, wait) { \
.func = H5##module##_term_package \
, .name = #module \
, .completed = false \
, .await_prior = wait \
}

/*
* Termination is ordered by the `terminator` table so the "higher" level
* packages are shut down before "lower" level packages that they
* rely on:
*/
struct {
int (*func)(void); /* function to terminate the module; returns 0
* on success, >0 if termination was not
* completed and we should try to terminate
* some dependent modules, first.
*/
const char *name; /* name of the module */
hbool_t completed; /* true iff this terminator was already
* completed
*/
const hbool_t await_prior; /* true iff all prior terminators in the
* list must complete before this
* terminator is attempted
*/
} terminator[] = {
/* Close the event sets first, so that all asynchronous operations
* complete before anything else attempts to shut down.
*/
TERMINATOR(ES, false)
/* Do not attempt to close down package L until after event sets
* have finished closing down.
*/
, TERMINATOR(L, true)
/* Close the "top" of various interfaces (IDs, etc) but don't shut
* down the whole interface yet, so that the object header messages
* get serialized correctly for entries in the metadata cache and the
* symbol table entry in the superblock gets serialized correctly, etc.
* all of which is performed in the 'F' shutdown.
*
* The tops of packages A, D, G, M, S, T do not need to wait for L
* or previous packages to finish closing down.
*/
, TERMINATOR(A_top, false)
, TERMINATOR(D_top, false)
, TERMINATOR(G_top, false)
, TERMINATOR(M_top, false)
, TERMINATOR(S_top, false)
, TERMINATOR(T_top, false)
/* Don't shut down the file code until objects in files are shut down */
, TERMINATOR(F, true)
/* Don't shut down the property list code until all objects that might
* use property lists are shut down
*/
, TERMINATOR(P, true)
/* Wait to shut down the "bottom" of various interfaces until the
* files are closed, so pieces of the file can be serialized
* correctly.
*
* Shut down the "bottom" of the attribute, dataset, group,
* reference, dataspace, and datatype interfaces, fully closing
* out the interfaces now.
*/
, TERMINATOR(A, true)
, TERMINATOR(D, false)
, TERMINATOR(G, false)
, TERMINATOR(M, false)
, TERMINATOR(S, false)
, TERMINATOR(T, false)
/* Wait to shut down low-level packages like AC until after
* the preceding high-level packages have shut down. This prevents
* low-level objects from closing "out from underneath" their
* reliant high-level objects.
*/
, TERMINATOR(AC, true)
/* Shut down the "pluggable" interfaces, before the plugin framework */
, TERMINATOR(Z, false)
, TERMINATOR(FD, false)
, TERMINATOR(VL, false)
/* Don't shut down the plugin code until all "pluggable" interfaces
* (Z, FD, PL) are shut down
*/
, TERMINATOR(PL, true)
/* Shut down the following packages in strictly the order given
* by the table.
/*
* Termination is ordered by the `terminator` table so the "higher" level
* packages are shut down before "lower" level packages that they
* rely on:
*/
, TERMINATOR(E, true)
, TERMINATOR(I, true)
, TERMINATOR(SL, true)
, TERMINATOR(FL, true)
, TERMINATOR(CX, true)
};

do {
pending = 0;
for (i = 0; i < NELMTS(terminator); i++) {
if (terminator[i].completed)
continue;
if (pending != 0 && terminator[i].await_prior)
break;
if (terminator[i].func() == 0) {
terminator[i].completed = true;
continue;
struct {
int (*func)(void); /* function to terminate the module; returns 0
* on success, >0 if termination was not
* completed and we should try to terminate
* some dependent modules, first.
*/
const char *name; /* name of the module */
hbool_t completed; /* true iff this terminator was already
* completed
*/
const hbool_t await_prior; /* true iff all prior terminators in the
* list must complete before this
* terminator is attempted
*/
} terminator[] = {
/* Close the event sets first, so that all asynchronous operations
* complete before anything else attempts to shut down.
*/
TERMINATOR(ES, false)
/* Do not attempt to close down package L until after event sets
* have finished closing down.
*/
, TERMINATOR(L, true)
/* Close the "top" of various interfaces (IDs, etc) but don't shut
* down the whole interface yet, so that the object header messages
* get serialized correctly for entries in the metadata cache and the
* symbol table entry in the superblock gets serialized correctly, etc.
* all of which is performed in the 'F' shutdown.
*
* The tops of packages A, D, G, M, S, T do not need to wait for L
* or previous packages to finish closing down.
*/
, TERMINATOR(A_top, false)
, TERMINATOR(D_top, false)
, TERMINATOR(G_top, false)
, TERMINATOR(M_top, false)
, TERMINATOR(S_top, false)
, TERMINATOR(T_top, false)
/* Don't shut down the file code until objects in files are shut down */
, TERMINATOR(F, true)
/* Don't shut down the property list code until all objects that might
* use property lists are shut down
*/
, TERMINATOR(P, true)
/* Wait to shut down the "bottom" of various interfaces until the
* files are closed, so pieces of the file can be serialized
* correctly.
*
* Shut down the "bottom" of the attribute, dataset, group,
* reference, dataspace, and datatype interfaces, fully closing
* out the interfaces now.
*/
, TERMINATOR(A, true)
, TERMINATOR(D, false)
, TERMINATOR(G, false)
, TERMINATOR(M, false)
, TERMINATOR(S, false)
, TERMINATOR(T, false)
/* Wait to shut down low-level packages like AC until after
* the preceding high-level packages have shut down. This prevents
* low-level objects from closing "out from underneath" their
* reliant high-level objects.
*/
, TERMINATOR(AC, true)
/* Shut down the "pluggable" interfaces, before the plugin framework */
, TERMINATOR(Z, false)
, TERMINATOR(FD, false)
, TERMINATOR(VL, false)
/* Don't shut down the plugin code until all "pluggable" interfaces
* (Z, FD, PL) are shut down
*/
, TERMINATOR(PL, true)
/* Shut down the following packages in strictly the order given
* by the table.
*/
, TERMINATOR(E, true)
, TERMINATOR(I, true)
, TERMINATOR(SL, true)
, TERMINATOR(FL, true)
, TERMINATOR(CX, true)
};

do {
pending = 0;
for (i = 0; i < NELMTS(terminator); i++) {
if (terminator[i].completed)
continue;
if (pending != 0 && terminator[i].await_prior)
break;
if (terminator[i].func() == 0) {
terminator[i].completed = true;
continue;
}

/* log a package when its terminator needs to be retried */
pending++;
nprinted = HDsnprintf(next, nleft, "%s%s",
(next != loop) ? "," : "", terminator[i].name);
if (nprinted < 0)
continue;
if ((size_t)nprinted >= nleft)
nprinted = HDsnprintf(next, nleft, "...");
if (nprinted < 0 || (size_t)nprinted >= nleft)
continue;
nleft -= (size_t)nprinted;
next += nprinted;
}
} while (pending && ntries++ < 100);

/* log a package when its terminator needs to be retried */
pending++;
nprinted = HDsnprintf(next, nleft, "%s%s",
(next != loop) ? "," : "", terminator[i].name);
if (nprinted < 0)
continue;
if ((size_t)nprinted >= nleft)
nprinted = HDsnprintf(next, nleft, "...");
if (nprinted < 0 || (size_t)nprinted >= nleft)
continue;
nleft -= (size_t)nprinted;
next += nprinted;
}
} while (pending && ntries++ < 100);

/* clang-format on */
/* clang-format on */

if (pending) {
/* Only display the error message if the user is interested in them. */
if (func) {
fprintf(stderr, "HDF5: infinite loop closing library\n");
fprintf(stderr, " %s\n", loop);
if (pending) {
/* Only display the error message if the user is interested in them. */
if (func) {
fprintf(stderr, "HDF5: infinite loop closing library\n");
fprintf(stderr, " %s\n", loop);
#ifndef NDEBUG
HDabort();
#endif /* NDEBUG */
} /* end if */
} /* end if */
HDabort();
#endif /* NDEBUG */
} /* end if */
} /* end if */
}

/* Free open debugging streams */
while (H5_debug_g.open_stream) {
Expand Down Expand Up @@ -1116,7 +1120,7 @@ H5close(void)
*
*-------------------------------------------------------------------------
*/
void *
void *H5_ATTR_MALLOC
H5allocate_memory(size_t size, hbool_t clear)
{
void *ret_value = NULL;
Expand Down
23 changes: 0 additions & 23 deletions src/H5Dmpio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,29 +48,6 @@
#define H5D_MULTI_CHUNK_IO 1
#define H5D_ONE_LINK_CHUNK_IO_MORE_OPT 2
#define H5D_MULTI_CHUNK_IO_MORE_OPT 3
#define H5D_NO_IO 4

/***** Macros for One linked collective IO case. *****/
/* The default value to do one linked collective IO for all chunks.
* If the average number of chunks per process is greater than this
* value, the library will create an MPI derived datatype to link all
* chunks to do collective IO. The user can set this value through an
* API.
*/

/* Macros to represent options on how to obtain chunk address for one linked-chunk IO case */
#define H5D_OBTAIN_ONE_CHUNK_ADDR_IND 0
#define H5D_OBTAIN_ALL_CHUNK_ADDR_COL 2

/* Macros to define the default ratio of obtaining all chunk addresses for one linked-chunk IO case */
#define H5D_ALL_CHUNK_ADDR_THRES_COL 30
#define H5D_ALL_CHUNK_ADDR_THRES_COL_NUM 10000

/***** Macros for multi-chunk collective IO case. *****/
/* The default value of the threshold to do collective IO for this
* chunk. If the average number of processes per chunk is greater
* than the default value, collective IO is done for this chunk.
*/

/* Macros to represent different IO modes(NONE, Independent or collective)for multiple chunk IO case */
#define H5D_CHUNK_IO_MODE_COL 1
Expand Down
2 changes: 2 additions & 0 deletions src/H5FDmpio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,8 @@ H5FD__mem_t_to_str(H5FD_mem_t mem_type)
return "H5FD_MEM_LHEAP";
case H5FD_MEM_OHDR:
return "H5FD_MEM_OHDR";
case H5FD_MEM_NTYPES:
return "H5FD_MEM_NTYPES";
default:
return "(Unknown)";
}
Expand Down
Loading