fix implicit overlapping move

[lefessan]

I ran into a small issue: READ FILE-FD INTO RECORD where RECORD is declared as the FD of FILE-FD with a numeric type will end up always reading zeroes (because of an overlapping move). The compiler already issues a warning, but I think it is still better to get the correct behavior when possible, so I modified the cob_move_alphanum_to_display to correctly handle overlaps (the extra cost is only paid in case of overlap).

[GitMensch]

Please check a bit deeper - the compiler should, depending on the dialect, generate either a move or a move_ibm, and the first should uses memcpy... or does this only happen if the target is numeric and the source isn't?

Is there still a different result depending on the move-ibm dialect option?

I think that should be also possible to reproduce without a file, just using a "normal" overlapping MOVE, no?

[lefessan]

I won't have time to investigate more, but:

• the program that I added in the testsuite is the simplified version of a program that I ran both on GnuCOBOL and MF, and it worked correctly on MF, but failed on GnuCOBOL (because all values were read as ZEROES);
• the problem is that a cob_move is created by the READ, and tries to translate from alphanum to display, causing the memset(0) of on the overlapping destination;
• the problem would indeed probably appear on a redefine of an alphanum into display, then moving the value from one representation to the other one inside the same memory location. Yet, I think such a program would probably be less likely to appear, than the case of a phantom move;

[engboris]

@GitMensch

To give more details, the program suggested by Fabrice is

       IDENTIFICATION DIVISION.
       PROGRAM-ID. prog.
       ENVIRONMENT DIVISION.

       INPUT-OUTPUT SECTION.
       FILE-CONTROL.
           SELECT MY-FILE ASSIGN TO "testfile"
               ORGANIZATION IS LINE SEQUENTIAL.

       DATA DIVISION.
       FILE SECTION.
       FD  MY-FILE.
       01  MY-REC PIC 9(5).

       PROCEDURE DIVISION.
          OPEN INPUT MY-FILE.
          READ MY-FILE INTO MY-REC.
          DISPLAY MY-REC.
          CLOSE MY-FILE.
          STOP RUN.

generating (for READ):

  /* Line: 15        : READ               : move.cob */
  cob_read_next (h_MY_FILE, NULL, 1);
  if (unlikely(cob_glob_ptr->cob_exception_code != 0))
  {
    /* USE PROCEDURE Default Error Handler */
    // ...
  }
  else
  {
    cob_move (&f_18, &f_17);
  }

with fields

static const cob_field_attr a_1 =	{0x21,   0,   0, 0x0000, NULL};
static const cob_field_attr a_2 =	{0x10,   5,   0, 0x0000, NULL};

static cob_field f_17	= {5, b_18, &a_2};	/* MY-REC */
static cob_field f_18	= {5, b_18, &a_1};	/* MY-FILE Record */

If I understand correctly, the problem (display of 00000 not occuring in the file) occurs because we are doing an "heterogeneous" overlapping move from an alphanumeric (content of the file) to a numeric (record). This doesn't occur if we have MY-REC PIC X(5). This also doesn't seem to occur with "normal" overlapping moves (actually I'm not sure to see how I can exactly reproduce what happens with files using only "normal" overlapping moves).

What do you think?

[GitMensch]

The first thing to verify is that instead of cob_move we generate cob_move_ibm here if the -fmove-ibm is used - because if it isn't then this is easy to fix. ... and as there's no conversion in this case, the result in this specific case should be "as expected" as well, no?

The second part is the generation of cob_move - as far as a see that's quite a specific case because

       FD  MY-FILE.
       01  MY-REC.
            03 MY-DATA    PIC 9(5).
....
          READ MY-FILE INTO MY-REC.

will again lead to the "expected" result - even without -fmove-ibm - correct?

The issue here is that cob_move is to general, we only want to adjust the specific fileio variants and also want to ensure -fmove-ibm is used as expected.

If this is the case then we can adjust the following in cb_emit_read():

	if (into) {
		// TODO: check if overlapping and if yes, after warning codegen cb_build_move_copy
		current_statement->handler3 = cb_build_move (rec, into);
	}

Note: we possibly have a similar problem the other way around, which we can easily handle specifically in cb_emit_rewrite and cb_emit_write (which should be really be refactored to call something like validate_and_prepare_write_rewrite() with the parameters of write) in

	if (from && (!CB_FIELD_P(from) || (CB_FIELD_PTR (from) != CB_FIELD_PTR (record)))) {
		// TODO: check if overlapping and if yes, after warning codegen cb_build_move_copy
		cb_emit (cb_build_move (from, record));
	}

As both are nearly identical this possibly can be done in a new build_implicit_file_move, which returns the cb_tree of the statement.

@engboris Will you inspect this more and possibly come up with a fix (and some refactoring)?

[engboris]

No cob_move_ibm generated with -fmove-ibm. Actually, the C file output is identical with and without this option. The only difference is that there's no warning with -fmove-ibm (even with -Wall).

Do you mean we're exepected to use -fmove-ibm in this case and that it should generate cob_move_ibm instead of cob_move ?

Will you inspect this more and possibly come up with a fix (and some refactoring)?

Yes, I'll take care of this PR.

[GitMensch]

Do you mean we're exepected to use -fmove-ibm in this case and that it should generate cob_move_ibm instead of cob_move ?

Yes, so maybe the overlap checking does not work correctly between "fd record" and "variable" (I haven't checked more details).

Will you inspect this more and possibly come up with a fix (and some refactoring)?

Yes, I'll take care of this PR.

Thanks, hope to review it tomorrow, if it would be already by then.

[engboris]

From what I see, cobc/typeck.c:cb_build_move_field is called with a source (src) of alphanumeric CB_TREE_CATEGORY. In this case, the function cb_build_move_copy (which is the only one able to output cob_move_ibm) is called only when the destination (dst) is also alphanumeric. Which is not the case, hence we fall back to a simple

return CB_BUILD_FUNCALL_2 ("cob_move", src, dst);

at the end of the function.

Here is the code of cb_build_move_copy:

static cb_tree
cb_build_move_copy (cb_tree src, cb_tree dst)
{
	int	size;

	size = cb_field_size (dst);
	if (size == 1) {
		return CB_BUILD_FUNCALL_2 ("$F", dst, src);
	}
	if (cb_move_ibm) {
		overlapping = 0;
		return CB_BUILD_FUNCALL_3 ("cob_move_ibm",
					   CB_BUILD_CAST_ADDRESS (dst),
					   CB_BUILD_CAST_ADDRESS (src),
					   CB_BUILD_CAST_LENGTH (dst));
	} else if (overlapping
	|| CB_FIELD_PTR (src)->storage == CB_STORAGE_LINKAGE
	|| CB_FIELD_PTR (dst)->storage == CB_STORAGE_LINKAGE
	|| CB_FIELD_PTR (src)->flag_item_based
	|| CB_FIELD_PTR (dst)->flag_item_based) {
		overlapping = 0;
		return CB_BUILD_FUNCALL_3 ("memmove",
					   CB_BUILD_CAST_ADDRESS (dst),
					   CB_BUILD_CAST_ADDRESS (src),
					   CB_BUILD_CAST_LENGTH (dst));
	} else {
		return CB_BUILD_FUNCALL_3 ("memcpy",
					   CB_BUILD_CAST_ADDRESS (dst),
					   CB_BUILD_CAST_ADDRESS (src),
					   CB_BUILD_CAST_LENGTH (dst));
	}
}

Do you have a suggestion @GitMensch?

[engboris]

I tried something very naive. It works but I'm not sure this is the right thing to do as I don't have the big picture (I'll add Changelogs after confirmation).