Small fixes from profiling PR

[lefessan]

Do not move object files if they were specified as an explicit target on the command line (typically cobc -c --save-temps=DIR -o foo.o foo.cob should keep foo.o in the current directory)

[GitMensch]

not sure about all of those, for the cobc change we should have a testcase after the commit

[GitMensch]

Do you have an example where this is probematic? It isn't if you use CBL_GC_FORK, of course. Note that (especially with 4.x) there are also other elements that need to be adjusted.

[lefessan]

No, except the use of fork() in external C code.

[GitMensch]

I'm not sure if/how we should handle that. The scenario's I think of are:

• "external" -> COBOL (then back, then fork then to COBOL in both)
• COBOL -> "external with fork" -> to COBOL

In both cases the runtime needs to know about that, because it needs to reset some things. While this is only the cached PID in GC 3.1+3.2, we may need more entries. For example trunk has improved locking with BDB and this is done internally by a "locker id" - and this may not be used in more than a single thread (or even process). fbdb keeps its locker ids and resets them (later acquiring a new one) for a cbl_sys_fork'd process.

So... can we handle that outside of cob_sys_fork? I currently see no option.
If there is no option, why should we support a specific case of fork that may won't work otherwise?

It seems better to explicit document that after the COBOL runtime is initialized you may only run cob_sys_fork() but not fork() directly. We can of course come up with an API function which we call from within cob_sys_fork() for childs and that external programs may use when they do a fork() before getting to COBOL again.

Opinions?

[lefessan]

Indeed, the only solution I see (unless we can detect that a fork happened at low cost) would be to provide a function cob_must_be_called_after_fork() to reset the process-specific state. Indeed, in the meantime, it's probably worth keeping cob_sys_getpid() (though there are still other places with getpid()).

[codecov-commenter]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c0d64ad) 65.74% compared to head (a07514b) 65.76%.

Files	Patch %	Lines
cobc/cobc.c	25.00%	0 Missing and 3 partials ⚠️
libcob/common.c	25.00%	2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #126      +/-   ##
=====================================================
+ Coverage              65.74%   65.76%   +0.02%     
=====================================================
  Files                     32       32              
  Lines                  59092    59095       +3     
  Branches               15575    15577       +2     
=====================================================
+ Hits                   38849    38865      +16     
+ Misses                 14262    14251      -11     
+ Partials                5981     5979       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lefessan]

@GitMensch Any reason why config.h is not included systematically in all other .h files ?

[GitMensch]

Yes, because this generated file has no inclusion guard - it should be included only once - and should be included before any system header in any case. As tarstamp.h may be used in there (that was kind of common for environments without auto-configuration in the patchlevel, but we could drop support for that) it is included before config.h

[lefessan]

Yes, config.h is generated, but some of the defined macros must be defined, so we could have:

#ifndef COBCRUN_NAME
#include "config.h"
#endif 

?

[GitMensch]

In the current code that would be

#ifndef COBCRUN_NAME
#ifndef COB_TAR_DATE
#include "tarstamp.h"
#endif
#include "config.h"
#endif

in both cobc/cobc.h and libcob/coblocal.h

I'd prefer the more easy one to just have

/* may be used in config.h, e.g. for PATCH_LEVEL */
#include "tarstamp.h"
/* (generated) configuration for this build */
#include "config.h"

in all source files as the first code line in the beginning.

... thinking of this again, tarstamp.h may only be used in config.h if it is manually created (like in build_windows/config.h.in), so we should just include it there and in the places using it (without checking: libcob/common.c, cobc/cobc.c, cobc/codegen.c); I'll do this change to the 3.x branch "soon".

Further: actually config.h should only be used if HAVE_CONFIG_H is defined "to be clean" but I think it isn't reasonable to expect anyone to pass that much defines on the command line, so plan to ignore this.

[lefessan]

I'd prefer the more easy one to just have in all source files as the first code line in the beginning.

I am not fond of such "informal convention", that may lead to bugs the next time somebody creates a new source file. Just during this PR, I had to debug weird behaviors twice, because of the position of config.h before or after other include files.

Another idea: create a header file cob-config.h that contains (or whatever code needed to load the config):

#ifndef COB_CONFIG_H
#ifndef COBCRUN_NAME
#ifndef COB_TAR_DATE
#include "tarstamp.h"
#endif
#include "config.h"
#endif
#endif

and have it included from all other include files, and replace config.h in source files.

[GitMensch]

... but even then this cob-config.h must either be included everywhere or included in coblocal.h and cobc.h, no? In this case it would be better placed into those files directly.

Note that 4.x has a new "private" compile-time header, we can include something like that there in any case.

[lefessan]

I simplified this PR to only fix the --save-temps=DIR behavior, and export cob_get_strerror in coblocal.h.

For the config.h, I am not sure that adding the check in cobc.h would be ok, as this file is usually included only very late in files, while config.h must be the first included files (even before system includes).

I was also surprised that the configuration is not exported during installation, I would have expected libcob/common.h to include config.h and have config.h found there too (it might be useful to check the config.h after installation to see what flags where used).

Anyway, I propose to have this discussion in another PR, maybe for 4.x, when we start targetting it for next changes. By the way, is there a thread somewhere on how the switch between 3.x to 4.x is organized ?

[GitMensch]

that's fine now; and as my work on svn finally started (with a lot of delay) this should soon be able to actually be included upstream