Add optional index check in 3.x #191

ddeclerck · 2024-10-09T10:34:54Z

This PR attemps to backport SVN commit 4953 4954 & 5358 to 3.x.

ddeclerck · 2024-10-09T10:36:59Z

cobc/typeck.c

+						if (CB_FIELD_PTR (sub)->flag_indexed_by) {
+							continue;
+						}
+					}
 					if (cb_subscript_check != CB_SUB_CHECK_MAX


That's where I'm wondering if the added lines above interact badly with this.

No need to wonder (my guess is this breaks cb_subscript_check: max) - but a reason to add this into the "enable / disable subscript check with ODO" test (run_subscripts.at) tests, which should get a prog2.cob in any case that uses an index in any case (which should behave identical for the current tests but likely break with the new option [which only makes sense in the prog2.cob test as it only applies to indexes).
... while you at this: please replace the leading tabs in that test's prog.cob.

Actually this was removed from trunk in the subsequent commit (4954).

tests/testsuite.src/run_misc.at

GitMensch

This is a new cobc option and should have an entry in the NEWS.

Your guess about the broken combination is likely correct and you'd need to adjust the new code to also include the new dialect option added in the meantime - but the (currently missing) extended test case will show if this is the case.

Note: please reword the help in flag.def, maybe something like "for index-names, don't apply EC-BOUND-SUBSCRIPT on subscript use per ISO rules but but on adjusting the index-name via SET or PERFORM statements, to reduce the amount of checks done".
I also wonder if we should place "index" into that option somewhere...

Please include into cobc/Changelog both Ron's original entry (with a remark "merged on YYYY-MM-DD as new option) and your on-top changes (the option and possibly inclusion into the dialect code) with that date.

GitMensch · 2024-10-10T15:52:06Z

cobc/typeck.c

+						if (CB_FIELD_PTR (sub)->flag_indexed_by) {
+							continue;
+						}
+					}
 					if (cb_subscript_check != CB_SUB_CHECK_MAX


No need to wonder (my guess is this breaks cb_subscript_check: max) - but a reason to add this into the "enable / disable subscript check with ODO" test (run_subscripts.at) tests, which should get a prog2.cob in any case that uses an index in any case (which should behave identical for the current tests but likely break with the new option [which only makes sense in the prog2.cob test as it only applies to indexes).
... while you at this: please replace the leading tabs in that test's prog.cob.

ddeclerck · 2024-10-10T16:24:24Z

Actually I'm not very confident I could do that quickly enough. I'd rather delegate this so I could focus on the GC3/GC4 merge - the clock's ticking and the days allocated to this task are almost depleted.

GitMensch · 2024-10-10T16:35:34Z

I was surprised you took on that backport. The issue that I've now realized is - if you want to merge the dialect option from GC3 to GC4 then this has to be done in any case :-/

ddeclerck · 2024-10-11T08:27:21Z

I was surprised you took on that backport. The issue that I've now realized is - if you want to merge the dialect option from GC3 to GC4 then this has to be done in any case :-/

True indeed ; that dialect option is commit 5087 (so this will be a problem in ~50 commits). Though I'm just realizing now that feature was implemented by Nicolas ; I'm gonna ask him for help.

GitMensch · 2024-10-21T14:36:52Z

MSVC CI should be fixed with updating from upstream (I've worked on that before my vacation).

Apart from that: What is the state of this PR?

ddeclerck · 2024-10-21T16:05:56Z

Apart from that: What is the state of this PR?

I'd stay "stale" - waiting for someone else to take over.
In the meantime I made PR #194 to (temporarily) disable that check in GC4 so that I could continue the merge (as discussed in #147 (comment)).

GitMensch

... oops, forget to send "submit"... so here is my (6 weeks old?) review, requesting changes.

tests/testsuite.src/syn_occurs.at

GitMensch · 2024-10-24T11:56:58Z

tests/testsuite.src/run_subscripts.at

+       PROCEDURE DIVISION.
+           MOVE 5 TO MAXIDX
+           SET NIDX TO IB1.
+           DISPLAY "Initial value: " NIDX.


this is unrelated - if we haven't tested that yet, then it should be a separate test (run once with std=default and -std=ibm and once with std=mf (output only "bigger than max" as the value will be different with 32/64 bit there)

tests/testsuite.src/run_subscripts.at

cobc/typeck.c

GitMensch

First: Sorry that the review took so long. I did not submit it before; but that second view now actually brought some more "content" in, so that's possibly not bad.

The non-standard check as possible performance boost while keeping checks enabled are not an actual part of this PR (so far).

Depending on how you want to go on, we could reduce this PR to syntax improvement (parser) and the additional useful warning - and drop the change top flag.def.
Then maybe create a follow-up PR which has the additional move to the non-standard RANGE check (or postpone this for later).

... or: work on this to get this "all in"; but that will definitely be more work for nowö.

cobc/typeck.c

libcob/common.h

tests/testsuite.src/syn_occurs.at

cobc/typeck.c

GitMensch · 2024-12-30T12:53:33Z

cobc/flag.def

@@ -186,6 +186,9 @@ CB_FLAG (cb_flag_stack_check, 1, "stack-check",
 	_("  -fstack-check         PERFORM stack checking\n"
 	  "                        * turned on by --debug/-g"))

+CB_FLAG (cb_flag_check_subscript_set, 1, "opt-check-subscript-set",
+	_("  -fopt-check-subscript-set  check subscript in PERFORM/SET"))


The current implementation has only the check for SET, not for PERFORM - has it?

The change in codegen.c impacts PERFORM UNTIL; I assume that's why PERFORM is mentioned here. however, that specific part in code-generation might be about internal SETs induced by PERFORM statements. If so, then indeed there's not real point in mentioning PERFORM here.

cobc/typeck.c

GitMensch · 2024-12-30T13:03:18Z

cobc/typeck.c

+					if (emit_exception) {
+						cb_emit (CB_BUILD_FUNCALL_1 ("cob_set_exception",
+									     cb_int (COB_EC_RANGE_INDEX)));
+					}


the code around this is definitely useful; the fixed non-standard EC-RANGE exception in the compiled code is only useful when we do the same for common SET (run-time values) and skip the subscript exception runtime check for use of indexes later

I'm not sure I fully understand this comment… not to mention what change would be needed in the code ;-)

(Apologies for the request for clarification: I'm just discovering this PR).

GitMensch · 2025-01-03T17:25:53Z

Just stumbled over this, maybe the parser change already handles the issues mentioned in https://sourceforge.net/p/gnucobol/bugs/228/ (or can be extended to that)?

nberth · 2025-01-14T13:24:19Z

Just stumbled over this, maybe the parser change already handles the issues mentioned in https://sourceforge.net/p/gnucobol/bugs/228/ (or can be extended to that)?

No it does not. Is it only about raising an error/warning in this case (maybe with/without a dialect option)?

ddeclerck mentioned this pull request Oct 9, 2024

CI for forward-porting GC3 patches to GC4 #147

Draft

ddeclerck commented Oct 9, 2024

View reviewed changes

tests/testsuite.src/run_misc.at Show resolved Hide resolved

ddeclerck requested a review from GitMensch October 10, 2024 09:14

GitMensch requested changes Oct 10, 2024

View reviewed changes

ddeclerck force-pushed the index_check branch 2 times, most recently from 02e51ce to e27d944 Compare October 14, 2024 10:29

ddeclerck changed the title ~~Add optional index check~~ Add optional index check in 3.x Oct 21, 2024

ddeclerck force-pushed the index_check branch from e27d944 to 20a071f Compare October 22, 2024 09:57

GitMensch mentioned this pull request Oct 23, 2024

"Fix" PERFORM bounds check in 4.x - but disable it for now as it is not ISO-compliant #194

Closed

ddeclerck force-pushed the index_check branch from 20a071f to e7c9540 Compare October 23, 2024 14:20

GitMensch requested changes Dec 6, 2024

View reviewed changes

ddeclerck and others added 2 commits December 17, 2024 13:20

Add optional index check

f20c582

Update tests and function name

82b76e4

emilienlemaire force-pushed the index_check branch from f44e6a6 to 82b76e4 Compare December 17, 2024 12:20

emilienlemaire requested a review from GitMensch December 17, 2024 12:25

GitMensch requested changes Dec 30, 2024

View reviewed changes

nberth added 2 commits January 14, 2025 11:48

Minor fixes

5c1246b

Remove extraneous header inclusion

6835149

nberth force-pushed the index_check branch from 0220711 to 6835149 Compare January 14, 2025 10:50

Add ChangeLog entries of back-ported commits

6e3f71d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional index check in 3.x #191

Add optional index check in 3.x #191

ddeclerck commented Oct 9, 2024 •

edited

Loading

ddeclerck Oct 9, 2024

GitMensch Oct 10, 2024

ddeclerck Oct 14, 2024

GitMensch left a comment

GitMensch Oct 10, 2024

ddeclerck commented Oct 10, 2024

GitMensch commented Oct 10, 2024

ddeclerck commented Oct 11, 2024

GitMensch commented Oct 21, 2024

ddeclerck commented Oct 21, 2024

GitMensch left a comment

GitMensch Oct 24, 2024

GitMensch left a comment

GitMensch Dec 30, 2024

nberth Jan 13, 2025

GitMensch Dec 30, 2024

nberth Jan 13, 2025

GitMensch commented Jan 3, 2025

nberth commented Jan 14, 2025

Add optional index check in 3.x #191

Are you sure you want to change the base?

Add optional index check in 3.x #191

Conversation

ddeclerck commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddeclerck commented Oct 10, 2024

GitMensch commented Oct 10, 2024

ddeclerck commented Oct 11, 2024

GitMensch commented Oct 21, 2024

ddeclerck commented Oct 21, 2024

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch commented Jan 3, 2025

nberth commented Jan 14, 2025

ddeclerck commented Oct 9, 2024 •

edited

Loading