Fixes for bug reported under MSVC Debug

[ddeclerck]

This PR fixes problems reported by the runtime error checker under MSVC Debug.

Most of them were fixed by being pedantic about casts with loss of data (as hinted by the error popups).

One of them (CURRENCY SIGN WITH PICTURE SYMBOL) was more serious: a stack corruption occurring because we were writing outside of an array. Actually SVN commit 4886 added a picture string symbol (U)but did not extend the relevant tables. I added one to the table size (cf tree.c), which is sufficient to avoid the corruption, however I'm not sure what data I should add to the precedence_table for this new picture string symbol (the patch itself only claims to add "minimal parsing support", so maybe it's just sufficient ?).

Only one issue remain with test "System routine CBL_GC_HOSTED", which fails because of a mismatch in the C runtime used by libcob and by custom C code (as the test itself claims: "test_stdio.c must be compiled with same C runtime as libcob to match").

[GitMensch]

Thanks you very much for inspecting this.

I'm confused about the alphabet parts, these should match into unsigned char for everything but national - and in this case we shouldn't cast to unsigned char... Similar for the digits. Can you elaborate on where this happens? I guess there's a different bug to solve there...

The picture was indeed a bug, please add U in the precedence table before N.

[ddeclerck]

I'm confused about the alphabet parts, these should match into unsigned char for everything but national - and in this case we shouldn't cast to unsigned char... Similar for the digits. Can you elaborate on where this happens? I guess there's a different bug to solve there...

At the point where the error is reported (typeck.c:3825, function cb_validate_collating), the node contains the following info:

high_val_char is indeed outside the range of an unsigned char.

The seems to match the changes made by SVN commit 5310 to test PROGRAM COLLATING SEQUENCE in syn_definition.at.

We probably need to have national-specific variants of cb_low and cb_high. This could also be an opportunity to have a look at #136, which deals with these HIGH/LOW-VALUES as well.

[GitMensch]

Yes, we need national variables instead of masking that error.

Is the digit one similar?

Am 26. August 2024 00:42:15 MESZ schrieb OCP David Declerck @.***>:

I'm confused about the alphabet parts, these should match into unsigned char for everything but national - and in this case we shouldn't cast to unsigned char... Similar for the digits. Can you elaborate on where this happens? I guess there's a different bug to solve there...

At the point where the error is reported (typeck.c:3825, function cb_validate_collating), the node contains the following info:

high_val_char is indeed outside the range of an unsigned char.

The seems to match the changes made by SVN commit 5310 to test PROGRAM COLLATING SEQUENCE in syn_definition.at.

We probably need to have national-specific variants of cb_low and cb_high. This could also be an opportunity to have a look at #136, which deals with these HIGH/LOW-VALUES as well.

-- Reply to this email directly or view it on GitHub: https://github.com/OCamlPro/gnucobol/pull/175#issuecomment-2309023166 You are receiving this because you commented.

Message ID: @.***>

[ddeclerck]

I adjusted the precedence table as needed.

For the digit case, this is really no big deal. The first error is on this line:

*q = (unsigned char) (*p << 4) + COB_D2I (*(p + 1));

Since bit-shifting promotes to int, indeed (*p << 4) may overflow an unsigned char, hence masking is requried. The second error is here:

*q = (unsigned char) (*p << 4) & 0xF0;

Same reason as above, so I added extra parentheses so that the masking occurs before the cast.

Yes, we need national variables instead of masking that error.

As this seems a bit more involved than a mere fixup, should I make a dedicated PR for this (or why not, include that in #136) ?

[GitMensch]

As 136 is not about national - please do an extra one (and if not done yet, drop that part from this PR).

I'll try to recheck this and the merge4 bunch during our longer lunch break (I'm on "holiday" - doing construction work - this week). Thank you for working on this!

[ddeclerck]

Dropped the alphabet parts from this PR.

[ddeclerck]

Please have a look at the seed part, then feel free to commit directly.

Is there anything wrong with my proposed solution ?

[GitMensch]

Please have a look at the seed part, then feel free to commit directly.

Is there anything wrong with my proposed solution ?

I don't think that unsigned long long will be correct for all these cases - the first is seed = (unsigned long)specified_seed; which should raise the debug error again if the seed is bigger than ULONG_MAX The next is the srand ((unsigned int)seed); for the legacy DISABLE_GMP_RANDOM case and the gmp_randseed_ui one for the other, which should raise the error if the seed is bigger than UINT_MAX/ULONG_MAX..

The idea is to always define the seed as cob_u64t - possibly better unsigned long (so no need for ifdefs there) and explicit mask when it is assigned (from cob_u64t to unsigned long int) and when used for calling into the legacy srand calls.

[ddeclerck]

The idea is to always define the seed as cob_u64t - possibly better unsigned long

Isn't cob_u64t better than unsigned long here ? (since sizeof(long) differs between Win64 & others)

[GitMensch]

The functions we call use unsigned long or int. And yes, that's "fun" with the different sizes.

[ddeclerck]

Hosted should work though as it should compile the C files using cobc which will use msvc, no? If that doesn't happen then an adjustment of the commands used should do the trick.

The thing is, even if libcob uses the debug C runtime, cobc will only link with that runtime if given the -g option. I was able to make the test pass by adding -g when compiling test_errno.c (but then of course it causes the test to fail in Release).

Would it make sense (at least for MSVC), to always link generated executables/modules with the same version of the C runtime as the one used by cobc/libcob ?

[GitMensch]

No, as this would mean a necessary relink by switching that. For now please add your note for both "still skipped" test in the CI definition (maybe just in #170) and the changes to GnuCOBOL from this one as-is upstream.

[ddeclerck]

Merged in SVN @ 5318.