OCamlPro / gnucobol

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

Check for incompatible data only when a receiver field is of category numeric in `MOVE` or `SET` #181

Closed nberth closed 2 months ago

nberth commented 2 months ago

This PR alters MOVE and SET statements to check for numerical data in emitter fields only when at least one receiver field is of category numeric. This essentially permits statements like MOVE SOME-NUMERIC TO SOME-ALPHANUM to succeed even when SOME-NUMERIC holds invalid data.


(Old description:)

On some systems (GCOS), checks for non-numeric data in numeric items on MOVE appear to be performed on receiver fields instead of emitters, whereas in GnuCOBOL the checks are always performed on emitters. However, the former behavior seems convenient for encoding "absence" of relevant values (or, in more modern terms, null-ness) by putting SPACES in the associated storage area. This notably allows coding patterns where a MOVE SOME-NUMERIC TO SOME-ALPHANUM is followed by a check for "nullness" with SOME-ALPHANUM IS EQUAL TO SPACES.

Although disabling the (fatal) exception EC-DATA-INCOMPATIBLE permits such a coding pattern, it also disables some other checks that are relevant.

Another possible workaround is to use the (somewhat obscure) -fcorrect-numeric. Yet, this "solution" currently alters emitter fields on MOVES (to put ZEROS instead of SPACES), and this is an often undesirable side-effet.

This PR currently identifies changes required to implement the check on receiver fiels instead of emitters. Given that this changes the underlying semantics of MOVE/SET (albeit in possibly marginal/rare cases — and this fails a NIST test apparently), one could make this subject to a dialect option.

GitMensch commented 2 months ago

Are you sure that GCOS checkls on the receiver side? That would be quite strange.

But one thing that could be useful is to only check on arithmetic statements, and never on MOVE/SET TO...

nberth commented 2 months ago

Are you sure that GCOS checkls on the receiver side? That would be quite strange.

But one thing that could be useful is to only check on arithmetic statements, and never on MOVE/SET TO...

From our tests we could identify that GCOS allows the aforementioned code pattern (MOVE from a numeric with invalid data to an alphanumeric item). Still, it errors with INVALID DECIMAL DATA on similar moves to a numeric item. My current interpretation is that checks are done on receiver fields, but another reason could be that checks for numeric are performed on MOVEs only when numeric conversions take place.

GitMensch commented 2 months ago

yes to the later part, the standard does say that no conversion (only padding/truncation) is done for "same usage", but I'm not 100% sure what that is supposed to mean...

nberth commented 2 months ago

yes to the later part, the standard does say that no conversion (only padding/truncation) is done for "same usage", but I'm not 100% sure what that is supposed to mean...

Further findings on the GCOS side:

ddeclerck commented 2 months ago

Is this okay to be merged @GitMensch ? (this is a critical issue for our customer)

GitMensch commented 2 months ago

Ops, I may have forgot something - the check is not correct...

GitMensch commented 2 months ago

Hm, can you please also have a look into the codegen and potentially cob_move? At least "if the target is alphanumeric, the source should be copied over verbatim (memcpy field->data + memcpy COB_ALPHANUMERIC_SPACE), no?

GitMensch commented 2 months ago

NEWS (+potentially Changedoc adjustment for the last push) would be missing

nberth commented 2 months ago

Hm, can you please also have a look into the codegen and potentially cob_move? At least "if the target is alphanumeric, the source should be copied over verbatim (memcpy field->data + memcpy COB_ALPHANUMERIC_SPACE), no?

Yes I think all these cases are handled in typeck.c:cb_build_move_copy via typeck.c:cb_build_move_field. cob_move seems to do the direct copy as well.

GitMensch commented 2 months ago

LGTM

nberth commented 2 months ago

Now upstream