Add COMP-5 binary size test

[ddeclerck]

This adds a test for COMP-5 binary size in presence of P.

[GitMensch]

That's good - let's expand this by adding the non P versions with expected same size and one run for each of the -fbinary-size options, then we have all things related to this in one single test run with different expected results.

Thanks again!

[ddeclerck]

Updated.

I'm a bit surprised by the results for -fbinary-size=2-4-8 though ; shouldn't V9(1) be two bytes with this setting ?

[GitMensch]

yes, so the testcase uncovered a bug (that trunk may not have any more...)

[ddeclerck]

Also present in GC4.

Apparently this is caused by this (field.c:compute_binary_size):

	case CB_BINARY_SIZE_2_4_8:
		if (f->flag_real_binary && size <= 2) {
			f->size = 1;
		} else {
			f->size = ((size <= 4) ? 2 :
				   (size <= 9) ? 4 : (size <= 18) ? 8 : 16);
		}

And this (field.c:validate_elementary_item):

	case CB_USAGE_COMP_5:
		f->flag_real_binary = 1;

I'd say it suffices to not set flag_real_binary when we have CB_USAGE_COMP_5 ?

But I'm also wondering, when flag_real_binary is set, why do we need to take the binary-size option into account ?

[ddeclerck]

I'd say it suffices to not set flag_real_binary when we have CB_USAGE_COMP_5 ?

But I'm also wondering, when flag_real_binary is set, why do we need to take the binary-size option into account ?

Actually nope, I understand we must keep flag_real_binary = 1 in validate_elementary_item.

However removing the specific case for f->flag_real_binary && size <= 2 in compute_binary_size fixes the problem, without breaking anything in the testsuite. But is it the right thing to do, @GitMensch ? Was this specific case added for a reason (not covered by the testsuite) ?

[GitMensch]

Rechecked (that complete check and specification below took more than two hours :-/ maybe I start to get old...): for 2-4-8 we should indeed never have a size of 1 (note: that setting is the GC3 implementation of MF's IBMCOMP directive, included in IBM + RM, but not in BS2000 [which does document 2-4-8-16 in its own documentation, a setting that is actually what our 2-4-8 means - we possibly should adjust the names in GC4...]).
Also checked Realia docs - the current 2-4-8 we have there is correct.

Attention: Per GCOS docs the right binary-size option would be 2-4-6-8, which is not the case (it is currently 2-4-8) - you may want to add that configuration option and setting!

Also: acu-strict.conf currently has binary-size: 1-2-4-8 # not verified yet - according to their docs (rule 8+9) that must be changed to 2-4-8-12-16 - if you add the GCOS one, please add this as well, if not please adjust it to the more correct 2-4-8.

So I've gone through the annotate/blame to check where the changes come from. For field.c that's May 2007, history:r2575

• field.c : BINARY-CHAR is always one byte regardless of -std=

That should obviously be the case and we should have a testcase for this...
But this seems to be not the case as you've said nothing breaks. Can you please add another test that verifies the BINARY-... types being the correct size [testing in COBOL with IF BYTE-LENGTH(var-char) <> 1 DISPLAY "BAD BINARY-CHAR"., then compile and run once with -fbinary-size=1--8 and once with -fbinary-size=2-4-8]).

And this case was, back then, totally correct, which should answer your question.

The real issue is the addition of the flag_real_binary to COMP-5, which is relatively new - from January 2017 code:r1372, merged in by me with r1423 in February 2017.

• field.c: Correction so that COB_FLAG_REAL_BINARY will be set on for COMP-5

This end result is correct (it is mostly about binary truncation instead of arithmetic one) but that actual change has an effect on the size generation as we found out - that it should not have for COMP-5! Even tree.h flag_real_binary tells us that...

To fix it (you know, when you work on the test cases I can focus on the code...)
adjust field.c:

 	case CB_USAGE_COMP_5:
-		f->flag_real_binary = 1;

and codegen.c:

-				if (f->flag_real_binary) {
+				if (f->flag_real_binary
+				 || f->usage == CB_USAGE_COMP_5) {
 					flags |= COB_FLAG_REAL_BINARY;
 				}
 				if (f->flag_is_pointer) {
 					flags |= COB_FLAG_IS_POINTER;
 				}
-				if (cb_binary_truncate &&
- 				    f->usage == CB_USAGE_BINARY &&
- 				    !f->flag_real_binary) {
+				if (cb_binary_truncate
+				 && f->usage == CB_USAGE_BINARY
+ 				 && !f->flag_real_binary) {
 					flags |= COB_FLAG_BINARY_TRUNC;
 					flags |= COB_FLAG_BINARY_TRUNC;
 				}
 
-				if (type == COB_TYPE_NUMERIC_BINARY
-				 && f->usage == CB_USAGE_INDEX) {
-					flags |= COB_FLAG_REAL_BINARY;
-					type = COB_TYPE_NUMERIC_COMP5;
-				} else
-				if (type == COB_TYPE_NUMERIC_BINARY
-				 && (f->flag_binary_swap || f->flag_real_binary)
-				 && (f->flag_indexed_by || f->index_type || f->flag_internal_register)) {
-					type = COB_TYPE_NUMERIC_COMP5;
+				if (type == COB_TYPE_NUMERIC_BINARY) {
+					if (f->usage == CB_USAGE_INDEX) {
+						flags |= COB_FLAG_REAL_BINARY;
+						type = COB_TYPE_NUMERIC_COMP5;
+					} else
+					if ((f->flag_binary_swap || f->flag_real_binary)
+					 && (f->flag_indexed_by || f->index_type || f->flag_internal_register)) {
+						type = COB_TYPE_NUMERIC_COMP5;
+					}
 				}

and of course Changelog:

2024-10-29  Simon Sobisch <[email protected]>

	* field.c (validate_elementary_item): only set flag_real_binary for
	  BINARY-CHAR/BINARY-SHORT/BINARY-LONG/BINARY-DOUBLE
	* codegenc (output_attr): generate COB_FLAG_REAL_BINARY also for COMP-5

and NEWS

+** #NNN: dialects without support of single byte binary data
+   will now generate at least 2 bytes of storage for COMP-5 elements; this
+   applies to all non-standard dialects but GnuCOBOL (default) and Micro Focus;
+   that changes the group length of records containing elements with less than
+   3 digits and also passes different sizes via CALL, which may need program
+   adjustments.
+
+** -std=acu / -std=acu-strict now generate BINARY and COMP-5 with at least
+   2 bytes of storage; the comment for #NNN above applies; if you want
+   to still use the sizes used since GnuCOBOL 2.2 with those dialects:
+   add -fbinary-size=1-2-4-8 to your compile options
+
 * Important Bugfixes

Which I think should both the existing testcases, the one you've added and the one new for BINARY-CHAR and friends.

If you prefer, I could also do this commit upstream.

[ddeclerck]

Attention: Per GCOS docs the right binary-size option would be 2-4-6-8, which is not the case (it is currently 2-4-8) - you may want to add that configuration option and setting!

Yeah, that's definately something we should consider (back then, we just chosed what was the "closest").

That should obviously be the case and we should have a testcase for this... But this seems to be not the case as you've said nothing breaks. Can you please add another test that verifies the BINARY-... types being the correct size [testing in COBOL with IF BYTE-LENGTH(var-char) <> 1 DISPLAY "BAD BINARY-CHAR"., then compile and run once with -fbinary-size=1--8 and once with -fbinary-size=2-4-8]).

Tests added ;)

And this case was, back then, totally correct, which should answer your question.

Yup, reading the code a bit more, I get why this was made this way.

Which I think should both the existing testcases, the one you've added and the one new for BINARY-CHAR and friends.

Yes, that seems to fix everything.

If you prefer, I could also do this commit upstream.

Well, I included your changes in this PR, so I can take care of that.

[GitMensch]

please include the last change as well: config/acu-strict.conf (+Changelog)

-binary-size:			1-2-4-8	# not verified yet
+binary-size:			2-4-8	# TODO: add 2-4-8-12-16

[ddeclerck]

Done.

Just realized I created the SVN ticket using a very old personal account of mine instead of my new "profesional" account - don't know if this can be changed...

[GitMensch]

The creator change would need an export, adjustment and re-import, so there would have to be a very convincing reason to do so. But I've adjusted the text and assigned the ticket to your new account.

BTW: congrats for this nice issue number :-)

[ddeclerck]

The creator change would need an export, adjustment and re-import, so there would have to be a very convincing reason to do so. But I've adjusted the text and assigned the ticket to your new account.

Alright.

BTW: congrats for this nice issue number :-)

Yeah, do I get a prize ? 😉

So, is this PR "ready for SVN" ?

[GitMensch]

I've started doc + testsuite adjustments, with the later resulting in realizing about a missing feature... If that's fine with you I'd do the upstream commit afterwards.

[ddeclerck]

I've started doc + testsuite adjustments, with the later resulting in realizing about a missing feature... If that's fine with you I'd do the upstream commit afterwards.

Alright, I leave it to you ;)