OCamlPro / gnucobol

A clone of the sourceforge GnuCOBOL compiler from COBOL to C.
https://get-superbol.com
GNU Lesser General Public License v3.0
16 stars 20 forks source link

Use inttypes.h and stdint.h where appropriate (fixes 920) #116

Closed ddeclerck closed 6 months ago

ddeclerck commented 9 months ago

I also had to handle cob_s64_t and cob_s64_t using stdint.h (when available), to silence spurious warnings (they were defined as long long int, but the PRId64 macro causes printf to expect long int on 64-bit Linux (long int and long long int are the same size on this platform, yet they are considered different types, hence warnings).

ddeclerck commented 9 months ago

Yet I'm not fully satisfied.

#define COB_S64_C(x)        x ## LL
#define COB_U64_C(x)        x ## ULL

Here, the suffix should depend on the actual type used to represent 64-bit integers (for instance long int on 64-bit Linux and long long int on Windows).

That needs more work.

GitMensch commented 9 months ago

Note, also @lefessan as the original issue is also about the UCRT used definitions, it would be good to adjust the Windows CI in msys2-setup to use a build matrix of at least one additional ucrt environment. I don't know how much those additional runs take, you may add all of the environments in the example https://github.com/marketplace/actions/setup-msys2#build-matrix

GitMensch commented 9 months ago

My findings on MINGW are identical. And as noted we can have two fallbacks, one for _WIN32 (as it's not) and the other for long long.

codecov-commenter commented 9 months ago

Codecov Report

Merging #116 (6c1a45a) into gcos4gnucobol-3.x (c0d64ad) will not change coverage. The diff coverage is 100.00%.

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

@@                Coverage Diff                 @@
##           gcos4gnucobol-3.x     #116   +/-   ##
==================================================
  Coverage              65.74%   65.74%           
==================================================
  Files                     32       32           
  Lines                  59092    59092           
  Branches               15575    15575           
==================================================
  Hits                   38849    38849           
  Misses                 14262    14262           
  Partials                5981     5981           
Files Coverage Δ
cobc/codegen.c 75.99% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ddeclerck commented 9 months ago

Took longer than expected, but I'm now satisfied. Compiles and runs the testsuite fine under Linux with GCC 11 and Clang 14 both with and without using the stdint/inttypes headers. Also compiles fine under Windows/Cygwin with GCC 11 and MinGW 11. However, I had a few failures on the testsuite, though these also occur without my patch. Although off-topic, my failures under Cygwin/GCC are tests number 20-22 and 24 (not sure why), and under MinGW they are tests number 8, 9, 20,22, 24, 30, 40, 43, 47, 50, 762, 771, 797-799, 806-809, 845 and a few others (most of these are likely due to improper configuration of some path variables, which I had trouble getting right: I had to manually fill COB_CONFIG_DIR, COB_COPY_DIR, COB_RUNTIME_DIR and C_INCLUDE_PATH with Windows-style paths, otherwise they would use Unix-style paths pointing in my Cygwin installation). Anyway I'll check these issues separately.

GitMensch commented 9 months ago

While still not finding the time to check-in some missing commits to svn... I'd like to try that tomorrow on old MinGW and old MSVC - can you please try with MSYS2 ucrt?

ddeclerck commented 9 months ago

While still not finding the time to check-in some missing commits to svn... I'd like to try that tomorrow on old MinGW and old MSVC - can you please try with MSYS2 ucrt?

Just finished testing: it almost works fine with MSYS2/UCRT (on Windows 10). I have a failure on test "771: direct CALL in from C w/wo error; no exit". Here is the log: testsuite_0771.log I'm investigating.

I also have 3 failed tests though, probably because of path-related issues, but those also fail without my patch:

Here are the logs, for reference: testsuite_0008.log testsuite_0808.log testsuite_0809.log

GitMensch commented 9 months ago

Thank you for your tests and further inspecting this.

Test 8 is rooted in TEMP being bad and (likely the C compiler after cobc?) breaking because of that.

Tests 808+809 are known false positives (MSYS2 executes the program with an absolute file name); it may be possible to set some MSYS environment variable to change that.

ddeclerck commented 9 months ago

771 is giving me a hard time. In fact it also occurs without my patch. The worst part is that it is totally random : if I run testsuite 771 in a loop, sometimes I get "ok", sometimes I get "FAILED"...

EDIT: The problem occurs specifically with STOP RUN (replacing by an EXIT program gives no error). Also, it does NOT occur if I define COB_WITHOUT_JMP. Investigating a bit, it seems this is because the COBOL module executing the STOP RUN statement is unloaded (with lt_dlclose) before cob_stop_run has finished its execution. This might be okay when calling exit, but when using longjmp, this probably messes up the stack frame. Since this seems only used with cob_call_with_exception_check, I'm tempted to suggest deferring the unloading of the currently running module just before returning from cob_call_with_exception_check. Or, why not, adding a pair of calls to lt_dlopen and lt_dlclose in cob_call_with_exception_check before setjmp and before return, so as to keep the module alive a bit longer.

But in any case, this is not related to this PR.

GitMensch commented 9 months ago

Ah, an edit... your analysis on 771 sounds very reasonable.

I'm not sure about the fix on this - before doing the longjmp we should do all the cleanup... Any further idea how to process? Note: we cannot do an dlopen in cob_call_with_exception_check as the program can be found in several places, including "already in memory from the last invocation". We possibly could do if (name) cob_resolve (name, 0, 0); as this ensures that the modules is loaded, the later cob_call will then get that. This increments the library counter also before setjmp which may be useful.

I try to come back to the actual PR content soon.