Skip to content

Conversation

@rountree
Copy link

@rountree rountree commented Nov 21, 2025

Fixes and updates to compile under gcc 15.1.0 with the flags -Wall, -Wextra, and -Werror.

Commits are based on the class of errors/warnings rather than files.

commit 47cb39:  Fixes three errors caused by stricter gcc defaults.
commit c6f85d:  Fixes one [-Wdiscarded-qualifiers] and one [-Wstringop-truncation].
commit a7260c:  Fixes fallthrough and initialization warnings.
commit acc263:  Fixes -Wsign-compare warnings.
commit 5182e8:  Fixes -Wformat warnings.
commit 32d0c6:  Suppresses -Wunused-parameter warnings by casting to void.

Fixes #94

@mplegendre
Copy link
Member

This looks like it needs a rebase. Existing commits from both Nick and I are in here with changed hashes.
How does this relate to your other warning cleanup?

@rountree
Copy link
Author

rountree commented Dec 1, 2025

I closed out the other warning cleanup PR; this PR replaces it. I'll get a rebase committed later this morning.

@rountree
Copy link
Author

rountree commented Dec 1, 2025

Just a single file needed cleanup. Compiles without errors or warning with -Wall -Wextra -Werror using gcc-15.1.0 on Tuolumne. All runTests passed.

@rountree rountree marked this pull request as ready for review December 1, 2025 23:24

typedef struct {
volatile int cur_id;
unsigned long locks[2];
Copy link
Member

@mplegendre mplegendre Dec 3, 2025

Choose a reason for hiding this comment

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

I'm nervous about changing the locks[] layout here. I don't remember why I wrote it this way, but I wouldn't have done something this odd without reason. Given that arrays and structs can have different packing (even if on x86_64 it shouldn't pack struct vs array different here), it might have had something to do with that. There's also some memory barriers around the code that uses these arrays, and there's always tough subtleties when those are involved.

That said, I can't re-determine why these should be arrays with just code inspection. And it's too bad I didn't comment why it's like this.

Given that this is a warning cleanup commit, let's not risk subtle semantic changes.

(And this applies to the two other array->struct conversions below as well)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Getting the #includes to work required updating a couple of Makefile.ams.

The source file is glibc/stdlib/canonicalize.c at git commit 1894e219dc530d7074085e95ffe3c1e66cebc072
*/

#pragma GCC diagnostic ignored "-Wsign-compare"
Copy link
Member

Choose a reason for hiding this comment

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

There are macros in src/utils/ccwarns.h for doing these

Copy link
Author

Choose a reason for hiding this comment

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

Those are handy. I've narrowed the range of the disabled warning to a single while loop. Putting the macro around the while test seems safe, but I'm not familiar with the rules of putting #praga within statements or across loop boundaries.

@mplegendre
Copy link
Member

Also, it looks like you did a merge to resolve the conflicts.
But it needs a rebase that adds only the warning commits. If you look at the commit list for the PR, there's several other commits that are already merged (but have different hashes in this PR).

gcc-15.1.0 throws an error on declaration/definition mismatches.

/p/vast1/rountree/repos/Spindle/src/client/biter/../../biter/linux_ids.c:27:14:
error: conflicting types for ‘biterc_get_rank’; have ‘unsigned int(int)’

/p/vast1/rountree/repos/Spindle/src/client/biter/../../biter/shmutil.c:242:6:
error: conflicting types for ‘update_shm_id’; have ‘void(shminfo_t *)’

/p/vast1/rountree/repos/Spindle/src/client/client/remap_exec.c:168:6:
error: conflicting types for ‘remap_executable’; have ‘void(int)’

Updates header files in each case.  Passes runTests.
Spindle/src/client/client/exec_util.c:201:21:
warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
Fixed by strdup-ing string to preserve constness of original.

Spindle/src/server/cache/../../utils/pathfn.c:61:4:
warning: ‘strncpy’ output may be truncated copying between 0 and 4096 bytes from a string of length 4096 [-Wstringop-truncation]
Code is correct but the compiler can't deduce that.  Replaced strncpy() with strdup()+snprintf().
https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8#forming_truncated_strings_with_strncat
Spindle/src/server/auditserver/ldcs_audit_server_handlers.c:1442:10:
warning: this statement may fall through [-Wimplicit-fallthrough=]
Solved with magic comment.
https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7

Spindle/src/fe/startup/config_mgr.cc:808:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]
Actual error, fixed by adding missing break.

Spindle/testsuite/symbind_test.c:310:7:
warning: missing initializer for field ‘matched’ of ‘test_bindings_t’ [-Wmissing-field-initializers]
Added missing NULL to intializaiton.
/p/vast1/rountree/repos/Spindle/src/client/biter/../../biter/shmutil.c:45:43:
Split out pid_t and locks into their own variables/arrays.
Affects lock_t, base_header_t, biter_header_t, and shmcache_header_t.

/p/vast1/rountree/repos/Spindle/src/client/biter/../../biter/demultiplex.c:197:17:
Changed type of msg_header_t.msg_target from uint32_t to int (src/biter/demultiplex.h)

/p/vast1/rountree/repos/Spindle/src/client/auditclient/writablegot.c:121:21:
Cribbed from earlier solution.

/p/vast1/rountree/repos/Spindle/src/client/client/../../utils/parseloc.c:250:15:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/client/client/../../utils/fileutil.c:84:25:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/client/client/remap_exec.c:94:33:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/client/client/intercept_readlink.c:147:12:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/client/client_comlib/client_api_socket.c:237:11:
Cribbed early solution.

/p/vast1/rountree/repos/Spindle/src/client/client_comlib/client_api.c:235:22:
Trickier than most, as the code is dependent on the size of the
variables in question.

/p/vast1/rountree/repos/Spindle/src/client/client/realpath.c:369:17:
In the interest of tracking the original glibc code, disable this warning.

/p/vast1/rountree/repos/Spindle/src/client/subaudit/update_pltbind.c:185:18:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/server/biter/../../biter/biterd.c:90:18:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/server/cache/global_name.c:71:11:
int->size_t

/p/vast1/rountree/repos/Spindle/src/server/cobo/../../cobo/cobo.c:477:25:
Replaced inet_addr() call with intel_aton() per man page recommendation.

/p/vast1/rountree/repos/Spindle/src/server/cobo/../../cobo/handshake.c:689:22: warning:
Used ssize_t to allow errors and data to share the same channel.

/p/vast1/rountree/repos/Spindle/src/server/comlib/ldcs_api_socket.c:291:11:
Duplicated code...

/p/vast1/rountree/repos/Spindle/src/server/comlib/ldcs_api_pipe.c: In function ‘ldcs_send_msg_pipe’:
Familiar issue of read/write returning ints and those sizes being stored in size_ts.

/p/vast1/rountree/repos/Spindle/src/server/auditserver/ldcs_elf_read.c: In function ‘readUpTo’:
Interesting that fread does not return an error condition, nor does it set errno.o

/p/vast1/rountree/repos/Spindle/src/server/auditserver/ldcs_audit_server_requestors.c: In function ‘get_requestor’:
Cribbed.

/p/vast1/rountree/repos/Spindle/src/server/auditserver/ldcs_audit_server_numa.c: In function ‘pattern_match’:
Cribbed.

src/server/auditserver/ldcs_audit_server_handlers.c
Nothing remarkable.

Passes runTests.
Rebased on devel.

Passes all runTests on tuolumne.

Note that there are configuration tests that are erroring out
due to -Werror.  Low priority, but probably worth fixing.

Need to rerun bootstrap due to Makefile.am changes.
@rountree
Copy link
Author

rountree commented Dec 5, 2025

I just noticed that -Wall -Wextra -Werror is causing failures in the configure process. These don't affect the results of runTests, but we might want to get those cleaned up.

We're now good to go with -Wall -Wextra -Werror.
@rountree rountree deployed to Spindle CI December 5, 2025 17:50 — with GitHub Actions Active
@mplegendre mplegendre merged commit 62ad270 into llnl:devel Dec 5, 2025
3 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.

Clean up warnings

3 participants