search

KineticPreProcessor / KPP

The KPP kinetic preprocessor is a software tool that assists the computer simulation of chemical kinetic systems
GNU General Public License v3.0
21 stars 11 forks source link

[FEATURE REQUEST] Cleanup of minor issues before 2.5.0 release #36

Closed yantosca closed 2 years ago

yantosca commented 2 years ago

@RolfSander has identified a few minor cleanup issues that should be tackled before releasing KPP 2.5.0:

The gcc still spits out a couple of warnings. It would be nice if we can get rid of them.

  • Agreed. Will have to look at the warnings to see what modifications to the C code we can make to stop them. I have already fixed a few warnings by removing unused variables, etc.

We have two Makefiles: One in the main directory and one inside src/. Do we really need both? I think it's fine to cd src first, and then call make. If you agree, I can move everything into src/.

We can do this but we need to make sure it doesn't break the C-I tests. I think the C-I tests assume that you build from the root folder. (Also tagging @laestrada)

There is a comment in .ci-pipelines/build-testing.yml: "Commits to feature branches do not trigger this pipeline". What is our policy here? Currently, branches called patch*, bugfix* and MECCA* are triggers. Is this what we want?

  • NOTE: I think this is because we adapted the build-testing.yml from GEOS-Chem, where we don't test feature branches but only when they are merged into dev. We can change this in the yml file.

    Can we delete util/mex.f? It is identical to mex.c and thus not a f77 file.

  • Yes, this can be deleted

I noticed that we now have both MAX_PATHLEN and MAX_PATH. Maybe these can be merged.

  • Yes, we can merge these

Also I will work on adding the documentation in ReadTheDocs format.

RolfSander commented 2 years ago

Another Makefile-related idea: KPP can produce Makefile.f90, Makefile.F90, Makefile.f, Makefile.c and Makefile.m. These are all files in Makefile syntax even though the suffix suggests otherwise. This is a bit confusing. What do you think about renaming them to Makefile_f90, Makefile_F90, Makefile_f, Makefile_c and Makefile_m? This could be done in code.c.

yantosca commented 2 years ago

Another Makefile-related idea: KPP can produce Makefile.f90, Makefile.F90, Makefile.f, Makefile.c and Makefile.m. These are all files in Makefile syntax even though the suffix suggests otherwise. This is a bit confusing. What do you think about renaming them to Makefile_f90, Makefile_F90, Makefile_f, Makefile_c and Makefile_m? This could be done in code.c.

I'm on it. Thanks for the suggestion.

yantosca commented 2 years ago

MAX_PATHLEN is removed (and folded into MAX_PATH) in 29fcb292ba8c4b98ab97ce622e8683824e1e1bf7

mex.f is removed in 34bfd725d11992863d8b21db3eea7643366c6c8b

C-I tests are now run for all branches in 984c6c4db5d12f185cac46c68414b3fe738d1746

yantosca commented 2 years ago

Makefile update in code.c now in commit 72450e09cf9b2e06097c911c402a2f562e9b51de

yantosca commented 2 years ago

Added fixes to Makefile update in c6885ea7946236f652a17b00400322ca1c6b16ba and c6885ea7946236f652a17b00400322ca1c6b16ba.

RolfSander commented 2 years ago

Just a question: If the Dockerfile has to replace the Intel compiler by gfortran, why isn't gfortran the default in the first place?

yantosca commented 2 years ago

@RolfSander that's a good question. We can make it be that.

Also -- I've pushed a bunch of fixes to the feature/cleanup branch. One of the big sources of warnings is when we do a self-referential sprintf (i.e. sprintf( str, "%s", str ); or something like that. Have been using strncat to replace as many of those as possible. Still more to do.

RolfSander commented 2 years ago

Thanks, this removed many of the warnings!

Is there a reason why you are using strncat instead of strcat?

Another question: If you want, I can start to merge the two KPP Makefiles into just one, i.e., into src/Makefile. Or do you prefer to do this yourself?

RolfSander commented 2 years ago

Long lists of products are now (hopefully) truncated correctly in kpp_monitor.f90: 8a941c7eda6ac5e5136fd8b8a3663898d44fc29e

@yantosca: sprintf(s, "") in gen.c is not a useless function :-) However, s[0] = '\0'; is probably better.

yantosca commented 2 years ago

@RolfSander: strncat is probably safer than strcat since it only will copy a fixed number of characters. See: https://stackoverflow.com/questions/6491038/strcat-vs-strncat-when-should-which-function-be-used. Also please go ahead and merge the 2 makefiles into one. I didn't get that far.

I also just pushed some more fixes that will remove warnings: 4e9ece7dae79afa669db873de9ebb5ca819d61c2

I'll also investigate the failed build.

yantosca commented 2 years ago

The C-I tests filed with:

This is KPP-2.5.0.

KPP is parsing the equation file.
KPP is computing Jacobian sparsity structure.*** stack smashing detected ***: terminated
Aborted (core dumped)
The command '/bin/sh -c /kpp/.ci-pipelines/ci-testing-script.sh' returned a non-zero code: 1
##[error]The command '/bin/sh -c /kpp/.ci-pipelines/ci-testing-script.sh' returned a non-zero code: 1
##[error]The process '/usr/bin/docker' failed with exit code 1
Finishing: Build image
yantosca commented 2 years ago

The stack smashing error may be due to an array going out of bounds. I will look into it soon.

RolfSander commented 2 years ago

strncat is probably safer than strcat since it only will copy a fixed number of characters. See: https://stackoverflow.com...

Thanks for the link! (I don't know what I would do without stackoverflow...)

RolfSander commented 2 years ago

please go ahead and merge the 2 makefiles into one.

Yes, I'll do that soon. To avoid additional complications, however, I will wait until the stack smashing error has disappeared.

yantosca commented 2 years ago

I think the issue is the sprintf statement in the code that you modified for the truncation. Will look at that as soon as I can.

RolfSander commented 2 years ago

Yes, it could be the code that I changed. Would strcpy(s, ""); be better than s[0] = '\0'; ?

I unfortunately cannot test this on my machine because the code runs fine here. Maybe it is Docker-related?

laestrada commented 2 years ago

We can do this but we need to make sure it doesn't break the C-I tests. I think the C-I tests assume that you build from the root folder. (Also tagging @laestrada)

Moving the Makefile should be fine -- we will just need to update the relevant line in the Dockerfile accordingly: eg:

RUN cd /kpp/src/ && make && chmod +x /kpp/.ci-pipelines/ci-testing-script.sh
yantosca commented 2 years ago

So the stack smashing error seems to be just a plain old buffer overflow. Increasing MAX_EQNLEN in gdata.h from 200 to 300 seems to fix it on my Linux laptop.

However, when I run in the container, I get a seg fault. Maybe hitting a memory limit. Might need to tweak that a little bit.

I have to move onto something else today but hope to get back to this soon.

yantosca commented 2 years ago

Pushed these fixes:

These commits passed the C-I test on Azure Dev Ops.

The seg fault on my own local Linux laptop may be because I haven't configured Docker with enough memory. I'll look into that.

Still on deck:

laestrada commented 2 years ago

Another Makefile-related idea: KPP can produce Makefile.f90, Makefile.F90, Makefile.f, Makefile.c and Makefile.m. These are all files in Makefile syntax even though the suffix suggests otherwise. This is a bit confusing. What do you think about renaming them to Makefile_f90, Makefile_F90, Makefile_f, Makefile_c and Makefile_m? This could be done in code.c.

On this note, with the naming scheme Makefile.f90 and Makefile.F90 or Makefile_f90 and Makefile_F90 there is no distinction between these files on case insensitive file systems (macOs by default). Eg. if you git clone on macos you get the following:

warning: the following paths have collided (e.g. case-sensitive paths
on a case-insensitive filesystem) and only one from the same
colliding group is in the working tree:

  'util/Makefile_F90'
  'util/Makefile_f90'

I would recommend some distinction between them eg. Makefile_upper_F90 and Makefile_lower_f90

yantosca commented 2 years ago

that's a good idea. I was running up against that on my mac. Will push that later.

RolfSander commented 2 years ago

Moving the Makefile should be fine -- we will just need to update the relevant line in the Dockerfile accordingly

Yes, I will take care of the Dockerfile.

Another question: Is it okay if I delete cflags.guess and cflags? I think these files are obsolete because we use Makefile.defs for machine-dependent settings now.

RolfSander commented 2 years ago

The seg fault on my own local Linux laptop may be because I haven't configured Docker with enough memory. I'll look into that.

@yantosca: I'm sorry I can't help you with this. I'm still unable to reproduce your error. Even when compiling with -Wall.

yantosca commented 2 years ago

Ok to remove cflags and cflags.guess, @RolfSander!

RolfSander commented 2 years ago

The revamped Makefile in src/ is now in the cleanup branch. Let me know in case you encounter any problems!

laestrada commented 2 years ago

The revamped Makefile in src/ is now in the cleanup branch. Let me know in case you encounter any problems!

Looks like it built successfully on azure, so we should be all set!

yantosca commented 2 years ago

Also FYI, the only compiler warnings we now get are:

gcc -g -Wall -I/usr/include -c lex.yy.c
lex.yy.c:1984:16: warning: ‘input’ defined but not used [-Wunused-function]
 1984 | #else
      |                ^    
lex.yy.c:1941:17: warning: ‘yyunput’ defined but not used [-Wunused-function]
 1941 | 
      |                 ^      

gcc -g -Wall -I/usr/include -c scanner.c
scanner.c: In function ‘AddMember’:
scanner.c:1116:39: warning: assignment to ‘char’ from ‘char *’ makes integer from pointer without a cast [-Wint-conversion]
 1116 |   crtMembers[ crtMemberNr ].coeff_str = nr;

gcc -g -Wall -I/usr/include -c y.tab.c
y.tab.c: In function ‘yyparse’:
y.tab.c:1415:16: warning: implicit declaration of function ‘yylex’ [-Wimplicit-function-declaration]
 1415 |       yychar = yylex ();
      |                ^~~~~

Not sure how to address all of these. I think the one with crtMembers just needs a cast to fix.

RolfSander commented 2 years ago

the only compiler warnings we now get are...

I do get a few more. I guess you'll get them as well when you run gmake maintainer-clean first.

code.c: In function ‘FlushBuf’:
code.c:210:3: warning: format not a string literal and no format arguments [-Wformat-security]
   fprintf( currentFile, outBuf );
   ^
code.c: In function ‘FlushThisBuf’:
code.c:222:3: warning: format not a string literal and no format arguments [-Wformat-security]
   fprintf( currentFile, buf );
   ^

Just replace fprintf by fprint?

gen.c: In function ‘GenerateMex’:
gen.c:3112:37: warning: format not a string literal and no format arguments [-Wformat-security]
     case F90_LANG: sprintf( suffix, f90Suffix ); break;
                                     ^

???

lex.yy.c:1962:17: warning: ‘yyunput’ defined but not used [-Wunused-function]
     static void yyunput (int c, char * yy_bp )
                 ^
lex.yy.c:2005:16: warning: ‘input’ defined but not used [-Wunused-function]
     static int input  (void)
                ^

I have no idea how to get rid of these warnings because the code in lex.yy.c was created automatically by flex.

scanner.c: In function ‘AddMember’:
scanner.c:1116:39: warning: assignment makes integer from pointer without a cast [-Wint-conversion]
   crtMembers[ crtMemberNr ].coeff_str = nr;
                                       ^

This is about FAMILIES. Maybe ask @msl3v for help.

y.tab.c: In function ‘yyparse’:
y.tab.c:1414:16: warning: implicit declaration of function ‘yylex’ [-Wimplicit-function-declaration]
       yychar = yylex ();
                ^

Another automatically generated file (from bison). Again, difficult to fix.

yantosca commented 2 years ago

@RolfSander, I added a function prototype to scan.h so that avoids the yylex() undefined function error.

The crtMembers warning I was able to fix with:

crtMembers[ crtMemberNr ].coeff_str = *nr;

since *nr is pointer to char but crtMembers[ crtMemberNr ].coeff_str is char.

yantosca commented 2 years ago

I just pushed fixes for fprintf and sprintf -- they needed a format string %s in between the 1st and 2nd arguments.

yantosca commented 2 years ago

I also just now pushed some more fixes for MacOS. So it seems that there is a hard cap on stack memory (see https://stackoverflow.com/questions/42315207/extend-the-stack-size-on-macos-sierra) so the best you can do is 65532 kb instead of setting it to unlimited. Therefore, I had to add an #ifdef MACOS statement so that we could set MAX_EQN and MAX_SPECIES to lower values than the defaults.

Good news, I was able to build KPP and run the C-I tests on my Macbook Air. So I think we have pretty much cleaned up everything. I think it's good to merge feature/cleanup into dev now.

I will work on the ReadTheDocs but I might not get to it for a day or so as I have some other things to attend to.

@msl3v: when you have the Forward Euler ready we can add that to this version as well.

RolfSander commented 2 years ago

Thanks, @yantosca, for the cleanup! The code compiles on my computer now w/o any warnings. I think we're ready for 2.5.0 now.

RolfSander commented 2 years ago

Also thanks for updating kpp.el! I forgot about that. I think I will add a new section for KPP developers to the User Manual, describing what you have to do to introduce a new command. Here is a quick summary:

yantosca commented 2 years ago

Thanks @RolfSander. I'll merge into dev and then open a new branch for the ReadTheDocs.

yantosca commented 2 years ago

Some updates:

  1. Added optional VdotOut to Fun() routine (similar to Aout)
  2. Started feature/ReadTheDocs branch. I've used Pandoc to convert the user manual but there will be some hand editing needed.
  3. I tried using code generated by the dev branch vs. code generated by KPP 2.3.3_gc in GEOS-Chem. We get identical results!