Quiet compiler warnings for registry.exe #1858

DWesl · 2023-05-08T16:49:28Z

Quiet the compiler warnings in registry.exe

TYPE: bug fix

KEYWORDS: compiler warnings, snprintf, initial values, string length

SOURCE: "Daniel Wesloh (Pennsylvania State University Department of Meteorology and Atmospheric Science)"

DESCRIPTION OF CHANGES:
Problem:
Generally or specifically, what was wrong and needed to be addressed?
A few years ago, registry.exe was segfaulting while compiling WRF. I enabled every compiler warning for that build process I could find, and tried to fix every one I saw. The problem ended up being insufficient stack space (fix merged a few years ago), but fixing the warnings probably won't hurt.

Solution:
What was down algorithmically and in the source code to address the problem?

make sure string buffers are large enough for the contents put in them
don't use larger types than necessary (i.e. use short instead of int for numbers in the 300--16,000 range, so printf knows that five characters will be enough)
Specify maximum lengths to copy using snprintf and friends
Make sure all variables are initialized.

ISSUE: None

LIST OF MODIFIED FILES: list of changed files

M       configure
M       doc/README.cygwin.md
M       inc/streams.h
M       tools/Makefile
M       tools/data.h
M       tools/fseek_test.c
M       tools/gen_allocs.c
M       tools/gen_args.c
M       tools/gen_config.c
M       tools/gen_defs.c
M       tools/gen_interp.c
M       tools/gen_irr_diag.c
M       tools/gen_scalar_indices.c
M       tools/gen_streams.c
M       tools/gen_wrf_io.c
M       tools/misc.c
M       tools/protos.h
M       tools/reg_parse.c
M       tools/registry.c
M       tools/registry.h
M       tools/set_dim_strs.c
M       tools/sym.c
M       tools/sym.h
M       tools/symtab_gen.c
M       tools/type.c

TESTS CONDUCTED:

Do mods fix problem? How can that be demonstrated, and was that test conducted?
- Quiet compiler warnings in registry DWesl/WRF#3
- WRF compile finishes with this patch
Are the Jenkins tests all passing?
- I have no idea how to run those

RELEASE NOTE: Eliminate some compiler warnings when building registry.exe

weiwangncar · 2023-05-09T19:07:03Z

@DWesl It looks like the compilation is failed for all of the tests. Our regression tests are run using gfortran compiler. Here is a particular error (also see the attached file):

make[2]: Entering directory '/wrf/WRF/tools'
gcc -DIWORDSIZE=4 -DMAX_HISTORY=25 -DNMM_CORE=0 -c -g registry.c
registry.c:7:11: fatal error: io.h: No such file or directory
7 | # include <io.h>
| ^~~~~~

output_3.txt

DWesl · 2023-05-09T20:55:58Z

Interesting. My own tests all used GFortran, so it's likely a Cygwin/Linux difference rather than a compiler one. I set up guards so the #include <io.h> should only get seen on Windows and Cygwin and not on Linux, so hopefully that problem goes away.

weiwangncar · 2023-05-09T23:48:55Z

OK, now our regression tests have passed:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

DWesl · 2023-05-10T00:59:37Z

OK, now our regression tests have passed:

As have mine on Cygwin (with #1812)

DWesl · 2023-08-13T17:51:26Z

Am I missing anything for this PR?

weiwangncar · 2023-10-02T19:29:54Z

@islas Are any of the changes proposed here in conflict with anything you've been working on?

islas · 2023-10-02T19:43:47Z

@weiwangncar Nope, no conflicts should arise from our respective changes in registry

weiwangncar · 2023-10-02T19:58:41Z

@islas Great! Should we consider this in develop or for 4.5.2?

islas · 2023-10-02T20:13:11Z

I'd say this would be a nice-to-have if we can get it in, but not critical.

weiwangncar · 2023-10-02T20:14:54Z

@islas Thanks. We can leave it on the develop branch.

tools/gen_allocs.c

islas · 2023-10-27T20:21:41Z

tools/gen_irr_diag.c

     strcat( line,piece );
   }
   strcat( line," /)\n" );
   fprintf( fp_inc,line );
   fprintf( fp_inc," \n");

-   for( i = 0; i < nChmOpts,rxt_cnt[i] > 0; i++ ) {
+   for( i = 0; /*i < nChmOpts &&*/ rxt_cnt[i] > 0; i++ ) {


It seems like this is relying on rxt_cnt to be default initialized to zero. I'm not too sure what it is used for, but given that all other loops use nChmOpts to specify number of indices into rxt_cnt, keeping this check seems prudent

The old version was ignoring the result of the nChmOpts comparison. Are you suggesting that check be included in the for-loop exit condition?

Ah! You're correct, I forgot that this use of the , operator is nulling and not &&

I'd imagine that the original intent was to have it included via && given the other loops. Maybe @weiwangncar or someone else with historic knowledge on this section of the Registry can chime in. Otherwise, as long as tests agree, I think that check should be there

Changed to the suggested form. I'd still like to get an opinion from someone who knows this bit of the code better than me.

tools/registry.c

tools/sym.c

DWesl

A few magic constants I didn't catch earlier, plus some possibly-premature optimizations.

DWesl · 2024-01-02T18:33:30Z

tools/gen_interp.c


            if ( p->ntl > 1 ) { sprintf(tag,"_2") ; sprintf(tag2,"_%d", use_nest_time_level) ; }
-            else              { sprintf(tag,"")   ; sprintf(tag2,"")                         ; }
+            else              { tag[0] = '\0'; tag2[0] = '\0'; }


sprintf will likely be clearer, and the compiler may notice this optimization anyway.

Alternately, strcpy/strncpy

DWesl · 2024-01-02T18:34:17Z

tools/gen_interp.c

@@ -200,7 +200,7 @@ gen_nest_interp1 ( FILE * fp , node_t * node, char * fourdname, int down_path ,
    if ( nest_mask & down_path )
    {
        if ( p->ntl > 1 ) { sprintf(tag,"_2") ; sprintf(tag2,"_%d", use_nest_time_level) ; }
-        else              { sprintf(tag,"")   ; sprintf(tag2,"")                         ; }
+        else              { tag[0] = '\0'; tag2[0] = '\0'; }


sprintf or strcpy will likely be clearer, and the compiler might notice this optimization anyway.

DWesl · 2024-01-02T18:34:49Z

tools/gen_interp.c

@@ -130,7 +130,7 @@ else if ( down_path[ipath] == FORCE_DOWN  ) { sprintf(halo_id,"HALO_FORCE_DOWN")
 else if ( down_path[ipath] == INTERP_UP   ) { sprintf(halo_id,"HALO_INTERP_UP") ; }
 else if ( down_path[ipath] == SMOOTH_UP   ) { sprintf(halo_id,"HALO_INTERP_SMOOTH") ; }
 sprintf(halo_define,"80:") ;
-sprintf(halo_use,"") ;
+ halo_use[0] = '\0' ;


sprintf may be clearer, and the compiler may still notice the optimization.

Or strcpy, since this isn't using any of the formatting features.

tools/gen_defs.c

tools/gen_config.c

tools/reg_parse.c

DWesl · 2024-01-02T19:07:05Z

tools/reg_parse.c

@@ -256,7 +275,7 @@ pre_parse( char * dir, FILE * infile, FILE * outfile )
 	        if ( !strcmp( tokens[F_USE] , tracers[i] ) ) found = 1 ; 
              }
 	      if ( found == 0 ) {
-	        sprintf(tracers[ntracers],tokens[F_USE]) ;
+	        snprintf(tracers[ntracers], 100, tokens[F_USE]) ;


Definition on line 89; should this be another magic constant?

DWesl · 2024-01-02T19:09:42Z

tools/registry.c

-#else
+#endif
 # include <sys/time.h>
 # include <sys/resource.h>


I suspect these two headers are still unavailable on Windows; the others might be.

DWesl · 2024-01-02T19:11:18Z

tools/registry.h

@@ -1,6 +1,12 @@
 #ifndef REGISTRY_H
+#include <stdlib.h>
+#include <ctype.h>
+#include <sys/unistd.h>


This might also be unavailable on Windows.

DWesl · 2024-01-02T19:16:00Z

tools/registry.c

@@ -157,7 +159,8 @@ main( int argc, char *argv[], char *env[] )
      sprintf( fname_wrk,"%s/Registry_irr_diag",dir ) ;
    }
 //  fprintf(stderr,"Registry tmp file = %s\n",fname_wrk);
-    sprintf(command,"/bin/cp %s %s\n",fname_in,fname_wrk);
+    /* we should be able to implement this using posix_spawn */
+    sprintf(command,"/bin/cp \'%s\' \'%s\'\n",fname_in,fname_wrk);


This will still break if the filename includes apostrophes, but I think that's unlikely enough to not try re-implementing Python's shlex.quote.

weiwangncar · 2024-01-03T11:55:03Z

@DWesl The compilation failed in our regression test. If you need to see the output, let me know.

weiwangncar · 2024-01-09T01:33:11Z

The regression test results from the last commit:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

weiwangncar · 2024-01-24T17:29:02Z

@islas Is this ready for approval?

islas · 2024-01-24T21:11:06Z

@weiwangncar I think @DWesl has a lot of self-marked revisions they are still going through. Is that correct?

DWesl · 2024-01-26T14:07:32Z

I completed all the simple work I noticed on a second passthrough. There's a few places where I would like feedback on style.

DWesl · 2024-01-26T14:20:56Z

Something that should be noted before the final merge: I accidently added a Cygwin workflow in GitHub actions; I'm not sure if the maintainers want to keep that in or out of the main repository.

EDIT: I added the entire "Fix Cygwin compile" PR to this one. Should I rebase the branch to drop those changes? Or just edit them out and let the merge sort out differences?

weiwangncar · 2024-02-01T02:49:38Z

@DWesl Thanks for removing the Cygwin changes!
@islas Are we ok with this PR?

islas · 2024-02-01T16:25:55Z

I still see some Cygwin changes to configure and the workflow. I'm unsure if that is something we will want to add, though having a test would be incredibly helpful. That content & discussion might be more suited to a Cygwin-specific PR

There might be more I wanted to add, but I don't remember. I'll have to add those the way I did the first time, compiling with -Werror until it works. This pass is almost exclusively focused on the registry. I'll have a few more for Fortran once I figure out how. Inspiration: registry was segfaulting, so I tried fixing the warnings in case it was one of those. It didn't work, but it's worth doing regardless.

Change compile flags back to what they were before.

Guard io.h #include to avoid Linux compilation failures.

Given how much space this provides at the moment, this had better not run over.

Most likely the better solution is to look up functions to copy or delete files directly. I'm not sure where to look for those, but I'm pretty sure they're not in the C standard.

I pushed it to another branch, so a PR version can get some eyes on it.

external/RSL_LITE implements these as int, so they need to be declared with int returns, not void.

Not quite all of them yet, but there's only one other, that one without an existing named constant. EDIT: There's about seventy left: I'll have to go through those again.

I think there's a way to pass this as a parameter to snprintf instead, but I don't remember what that is at the moment.

I looked up how to do this. Apparently you just stick a star for the width and put the variable with the width before the variable you want to have that width.

grep -E -e '\[[[:digit:]]{2,}\]' *.[CcHhFf]*

Lots of unused variables, but I should be able to leave those for a bit. Unused parameters would be a pain, so I'm leaving those.

DWesl · 2024-02-11T17:38:11Z

I split off the remaining Cygwin-specific fixes to DWesl#5. This should be just the bits that address warnings, with comments on the bits I thought might be controversial.

I wanted to make it a short to save space in the strings based on this, but given the push to optimize for clarity that's less relevant.

weiwangncar · 2024-02-22T17:08:34Z

@DWesl This PR has code conflicts with what is in the latest repository. Can you resolve these conflicts while preserving the latest change in the repository?

DWesl · 2024-02-24T17:09:42Z

There's a number of places that change !(p->node_kind & BIT_MASK) to (!p->node_kind) & BIT_MASK, which is basically always going to return false. Could you point me to the PR that made that change and the reasoning for it? My inclination otherwise would be to revert that change or to make it (~p->node_kind & BIT_MASK).

DWesl · 2024-02-24T17:25:18Z

Apparently that's #1942, which also attempted to fix warnings, in this case a warning that ! p->node_kind & BIT_MASK wasn't doing what was most likely wanted (extracting the bits corresponding to the mask then inverting the result), but was instead applying a logical-not to a bit sequence (which results in either 1 or 0) and then applying the bit mask (here 8, so the result would always be zero). Should I go through and change those to be either a bitwise-invert or apply the parentheses implied by the warning (so the bit-mask gets applied before the logical-not)?

The other option would be to just delete the conditional, but the comment implies there's a point to it. I just asked on that PR for clarification of what I should do with those cases.

…warnings-registry Fix merge conflicts.

I made nChmOpts a short int to keep buffer size down. Given the change to named constants, this is likely irrelevant. On the other hand, it may currently max out at five.

DWesl · 2024-03-02T16:54:37Z

Discussion resolved there, conflicts resolved here: I kept the bit-mask then logical-not I already had.

DWesl · 2024-04-08T16:31:46Z

Should I try to squash the changes into fewer commits?

weiwangncar · 2024-04-08T23:34:59Z

@islas Can you see if this PR can be accepted?

DWesl requested review from a team as code owners May 8, 2023 16:49

weiwangncar changed the base branch from master to develop October 2, 2023 20:15

weiwangncar added the Develop Branch label Oct 2, 2023

islas reviewed Oct 2, 2023

View reviewed changes

tools/gen_allocs.c Outdated Show resolved Hide resolved

islas reviewed Oct 27, 2023

View reviewed changes

DWesl commented Jan 2, 2024

View reviewed changes

DWesl added 4 commits February 8, 2024 12:28

STY: Create macro for extra space in destination buffer.

9e252e5

BLD,BUG: Don't add warning flags to default compilation

334a1a2

Change compile flags back to what they were before.

BUG: io.h only available on Windows/Cygwin

afd1c1a

Guard io.h #include to avoid Linux compilation failures.

DWesl added 9 commits February 8, 2024 12:34

STY: Add sufficient space to more destination buffers.

173864f

Given how much space this provides at the moment, this had better not run over.

ENH: Include the nChmOpts check in the loop condition in gen_irr_diag.c

ba2eb1c

STY: Delete the partial posix_spawn calls.

22c4ba0

Most likely the better solution is to look up functions to copy or delete files directly. I'm not sure where to look for those, but I'm pretty sure they're not in the C standard.

STY: Remove the partial memory leak fix.

1208434

I pushed it to another branch, so a PR version can get some eyes on it.

BUG: Declare gen_packs and gen_packs_halo return types as int

317d219

external/RSL_LITE implements these as int, so they need to be declared with int returns, not void.

STY: Replace more integer literals with named constants.

8bdb29d

Not quite all of them yet, but there's only one other, that one without an existing named constant. EDIT: There's about seventy left: I'll have to go through those again.

STY: Use TABLE_ENTRY for the max length of the KPP names

d51f4ca

I think there's a way to pass this as a parameter to snprintf instead, but I don't remember what that is at the moment.

STY: Pass number of characters in KPP name as printf argument

c116e16

I looked up how to do this. Apparently you just stick a star for the width and put the variable with the width before the variable you want to have that width.

STY: Two more file-path variables using NAMELEN.

4ba1aab

grep -E -e '\[[[:digit:]]{2,}\]' *.[CcHhFf]*

DWesl force-pushed the quiet-compiler-warnings-registry branch from 1d09961 to 4ba1aab Compare February 8, 2024 17:47

STY: combine fprintf calls, replace sprintf with strcpy.

764c90d

Lots of unused variables, but I should be able to leave those for a bit. Unused parameters would be a pain, so I'm leaving those.

STY: Keep nChmOpts an int

894373e

I wanted to make it a short to save space in the strings based on this, but given the push to optimize for clarity that's less relevant.

DWesl added 4 commits February 29, 2024 08:11

Merge remote-tracking branch 'wrf-model/develop' into quiet-compiler-…

75742b9

…warnings-registry Fix merge conflicts.

STY: Ensure line has enough space for all table entries.

77fbd16

STY: Revert narrowing of nChmOpts.

6dc8eb0

I made nChmOpts a short int to keep buffer size down. Given the change to named constants, this is likely irrelevant. On the other hand, it may currently max out at five.

Merge changes from GitHub into local changes.

6c12eee

Quiet compiler warnings for registry.exe #1858

Are you sure you want to change the base?

Quiet compiler warnings for registry.exe #1858

Conversation

DWesl commented May 8, 2023 • edited

weiwangncar commented May 9, 2023

DWesl commented May 9, 2023

weiwangncar commented May 9, 2023

DWesl commented May 10, 2023

DWesl commented Aug 13, 2023

weiwangncar commented Oct 2, 2023

islas commented Oct 2, 2023

weiwangncar commented Oct 2, 2023

islas commented Oct 2, 2023

weiwangncar commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DWesl Nov 4, 2023 • edited

Choose a reason for hiding this comment

DWesl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DWesl Jan 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwangncar commented Jan 3, 2024

weiwangncar commented Jan 9, 2024

weiwangncar commented Jan 24, 2024

islas commented Jan 24, 2024

DWesl commented Jan 26, 2024 • edited

DWesl commented Jan 26, 2024 • edited

weiwangncar commented Feb 1, 2024

islas commented Feb 1, 2024

DWesl commented Feb 11, 2024

weiwangncar commented Feb 22, 2024

DWesl commented Feb 24, 2024

DWesl commented Feb 24, 2024 • edited

DWesl commented Mar 2, 2024 • edited

DWesl commented Apr 8, 2024

weiwangncar commented Apr 8, 2024

DWesl commented May 8, 2023 •

edited

DWesl Nov 4, 2023 •

edited

DWesl Jan 2, 2024 •

edited

DWesl commented Jan 26, 2024 •

edited

DWesl commented Jan 26, 2024 •

edited

DWesl commented Feb 24, 2024 •

edited

DWesl commented Mar 2, 2024 •

edited