fix profiling of EVALUATE by lefessan · Pull Request #208 · OCamlPro/gnucobol

lefessan · 2025-01-03T11:39:46Z

Try to fix issue reported in #110 (comment) and meyfa/CobolCraft#214 (comment).

GitMensch · 2025-01-03T14:37:28Z

please add the test case from the original example to the testsuite, then that's good to upstream

cobc/typeck.c

lefessan · 2025-01-03T14:43:21Z

I added the test, and also fixed the same code for break, though I don't have a test case for that one...

tests/testsuite.src/run_misc.at

lefessan · 2025-01-03T14:54:55Z

Is it still ok for SVN with the change to the break function ?

GitMensch · 2025-01-03T15:04:31Z

I'm not sure yet to get the complete picture,

What are the elements of the chain(s) with and what without -fperf?
How does this change if we actually have a GO TO at the last position of the imperative statements of a WHEN?

[I'll check fir the break part in the meantime]

lefessan · 2025-01-03T16:07:20Z

For example, in the test, the chain would be [ CALL TestRecurse ] in the non-profiling case, and [ C_CALL "prof_begin_call" ; CALL TestRecurse ; C_CALL "prof_end_call" ] in the profiling case. The C_CALL are not "CB_STATEMENT_P", so the previous test was failing and behaving the same as if it was a GOTO, because the test was not correctly written.

I think that, in the case of a GOTO, profiling does not add any information before and after (it adds information to the GOTO itself), so it should be the same as without profiling.

GitMensch · 2025-01-03T16:17:01Z

Updated testcase:

       IDENTIFICATION DIVISION.
       PROGRAM-ID. Main.
       
       DATA DIVISION.
       WORKING-STORAGE SECTION.
           01 STR PIC X(6) VALUE "..-..-".
           01 OFFSET BINARY-LONG UNSIGNED.
       
       PROCEDURE DIVISION.
           MOVE 1 TO OFFSET
           PERFORM UNTIL OFFSET > LENGTH OF STR
               CALL "TestRecurse" USING STR OFFSET
           END-PERFORM.
       
       END PROGRAM Main.
       
       
       IDENTIFICATION DIVISION.
       PROGRAM-ID. TestRecurse IS RECURSIVE.
       
       DATA DIVISION.
       LINKAGE SECTION.
           01 LK-STR.
              03 LK-CHR PIC X OCCURS 6
        >> IF BREAK IS DEFINED
                 INDEXED BY LK-IDX
        >> END-IF

                 .
           01 LK-OFFSET BINARY-LONG UNSIGNED.
       
       PROCEDURE DIVISION USING LK-STR LK-OFFSET.
           DISPLAY "LK-OFFSET: " LK-OFFSET
        >> IF BREAK IS DEFINED
           SET LK-IDX TO LK-OFFSET
           SEARCH LK-CHR
               AT END
                   IF LK-OFFSET > 20
                      GOBACK
                   END-IF
                   ADD 1 TO LK-OFFSET
                   CALL "TestRecurse" USING LK-STR LK-OFFSET
               WHEN LK-CHR (LK-IDX) = "."
                   ADD 1 TO LK-OFFSET
           END-SEARCH.
        >> ELSE
           EVALUATE LK-STR(LK-OFFSET:1)
               WHEN "."
                   ADD 1 TO LK-OFFSET
                   CALL "TestRecurse" USING LK-STR LK-OFFSET
               WHEN OTHER
                   ADD 1 TO LK-OFFSET
           END-EVALUATE.
        >> END-IF
       
       END PROGRAM TestRecurse.

compile with -DBREAK for the second testcase (which actually breaks at runtime because of memory issues with current codegen).

maybe rename the testcase as "profiling and codegen" or something similar, with keywords EVALUATE and SEARCH.

Please add both expected profiling results to the testcase as well.

lefessan · 2025-01-03T16:33:57Z

In your example, where is the break supposed to be inserted ? In the WHEN of the SEARCH ?

Note that on my computer, this program with -DBREAK fails to compile without profiling, but works ok with profiling...

GitMensch · 2025-01-03T17:18:24Z

The break is not inserted with the current svn version:

        /* Line: 42        : CALL               : prof.cob */
#line 42 "prof.cob"
        cob_nop ();
#line 448 "prof.c"
        cob_procedure_params[0] = COB_SET_DATA (f_36, b_36);
        cob_procedure_params[1] = COB_SET_DATA (f_39, b_39);
        cob_glob_ptr->cob_call_params = 2;
        cob_glob_ptr->cob_stmt_exception = 0;
        if (unlikely((cob_glob_ptr->cob_exception_code & 0x0b00) == 0x0b00)) cob_glob_ptr->cob_exception_code = 0;
                if (unlikely(call_TestRecurse.funcvoid == NULL || cob_glob_ptr->cob_physical_cancel == 1))
        {
          call_TestRecurse.funcvoid = cob_resolve_cobol ("TestRecurse", 0, 1);
        }
        b_21 = ((int (*)(void *, void *))call_TestRecurse.funcint) (b_36, b_39);
        cob_prof_exit_procedure (prof_info, 3);
      }

      /* Line: 43        : WHEN               : prof.cob */

without:

        /* Line: 42        : CALL               : prof.cob */
#line 42 "prof.cob"
        cob_nop ();
#line 425 "prof.c"
        cob_procedure_params[0] = COB_SET_DATA (f_36, b_36);
        cob_procedure_params[1] = COB_SET_DATA (f_39, b_39);
        cob_glob_ptr->cob_call_params = 2;
        cob_glob_ptr->cob_stmt_exception = 0;
        if (unlikely((cob_glob_ptr->cob_exception_code & 0x0b00) == 0x0b00)) cob_glob_ptr->cob_exception_code = 0;
                if (unlikely(call_TestRecurse.funcvoid == NULL || cob_glob_ptr->cob_physical_cancel == 1))
        {
          call_TestRecurse.funcvoid = cob_resolve_cobol ("TestRecurse", 0, 1);
        }
        b_21 = ((int (*)(void *, void *))call_TestRecurse.funcint) (b_36, b_39);
        break;
      }

      /* Line: 43        : WHEN               : prof.cob */

GitMensch · 2025-01-03T17:58:26Z

If the program fails to compile, then please recode it to just do some addition in the sub-program no recursive calls (at least that would be my guess for the error). This also helps to keep that test to be more focused.

rechecked: The compile error got in with GnuCOBOL 3.2 (3.1.2 compiles it clean, 3.3 dev doesn't) - that should get a new upstream bug report (and be fixed until 3.3 final, rc1 would be nice, but not necessary as the error was already released).

Fix a bug where INDEXED BY in LINKAGE SECTION would cause the variable not to be added to the local include file, causing a compile error of the C code.

lefessan · 2025-01-06T23:05:51Z

I think I found the bug with the program: the INDEXED BY appearing in LINKAGE SECTION was not correctly handled by gnucobol, I had to modify parser.y to set its index_type to CB_INT_INDEX, as if it were defined in the LOCAL-STORAGE section.

GitMensch · 2025-01-07T12:07:35Z

Thanks for the analysis and fix, which are correct.
I've missed this back 2022-12-30

parser.y (occurs_index): use CB_INT_INDEX for LOCAL-STORAGE indexes

Please do the following adjustments to the tests and we should be able to upstream this today:

one new test for INDEXED BY in LINKAGE (keywords runmisc OCCURS SEARCH) - this verifies the second bug being fixed
adjust profiling tests - dropping the "cobc" keyword
one new profiling test (next to the others that are related) that CALLs a RECURSIVE program containing and incrementing a counter until it is 5, with the profile result checked
use the test in the current version but have a plain EVALUATE and SEARCH to test the break stuff (no need for a recursive program call), moved below the existing tests, checking the profile result

lefessan · 2025-01-07T15:21:33Z

I may be able to do the first 2 items tonight, but given my current agenda for this week and the next ones, I won't have time to do more...

GitMensch · 2025-01-07T19:04:59Z

OK, let's split the effort, you do what you can and post a note for others to take over.
... I'm checking the INDEXED BY differences between MF und IBM in the meantime.

emilienlemaire · 2025-01-23T12:41:54Z

Hello @GitMensch,
I've updated the tests, although I'm not sure what you're asking for in the last bullet point from your previous message.
I tried including the profiler's result in reproducible way for the CI to work

GitMensch

Sorry for the late getting back to that.
Testsuite needs adjustment, code itself is fine.

GitMensch · 2025-04-15T16:10:08Z

tests/testsuite.src/run_misc.at

+       END PROGRAM TestRecurse.
+])
+
+AT_CHECK([$COMPILE --free prog.cob])


no need for --free here (and if, then we should use the "new" variant with free as option), same below

GitMensch · 2025-04-15T16:11:40Z

tests/testsuite.src/run_misc.at

+           01 LK-OFFSET BINARY-LONG UNSIGNED.
+
+       PROCEDURE DIVISION USING LK-STR LK-OFFSET.
+           DISPLAY "LK-OFFSET: " LK-OFFSET


let's just display 1 + 11 to make that more concise, also adjust the condition for > 10

GitMensch · 2025-04-15T16:14:31Z

tests/testsuite.src/run_misc.at

+                      GOBACK
+                   END-IF
+                   ADD 1 TO LK-OFFSET
+                   CALL "TestRecurse" USING LK-STR LK-OFFSET


no need for recursive calls here, we do that in the next test

GitMensch · 2025-04-15T16:15:51Z

tests/testsuite.src/run_misc.at

+       END PROGRAM recurse.
+])
+
+AT_CHECK([$COMPILE --free -fprof prog.cob])


same as above, no need for free, less output if we only display 1 + 5 (maybe even WITH NO ADVANCING and no text...)

GitMensch · 2025-04-15T16:17:21Z

tests/testsuite.src/run_misc.at

+AT_SETUP([INDEXED BY in LINKAGE])
+
+AT_KEYWORDS([runmisc OCCURS SEARCH])


get those lines together and add the keyword "profiling"

GitMensch · 2025-04-15T16:17:52Z

tests/testsuite.src/run_misc.at

+AT_SETUP([profiling and codegen])
+AT_KEYWORDS([EVALUATE])


Shouldn't that be AT_SETUP([profiling EVALUATE])?
... and possibly most of those test include the keyword runmisc?

GitMensch · 2025-04-15T16:19:53Z

tests/testsuite.src/run_misc.at

+PROCEDURE DIVISION.
+    MOVE 1 TO OFFSET
+    PERFORM UNTIL OFFSET > LENGTH OF STR
+        CALL "TestRecurse" USING STR OFFSET


I guess one recursive test is enough, if this test should also have it then move the "simple recursive call" up front

lefessan force-pushed the z-2025-01-03-fix-profile-evaluate branch from 93347f5 to 1d7aadb Compare January 3, 2025 13:46

GitMensch reviewed Jan 3, 2025

View reviewed changes

cobc/typeck.c Outdated Show resolved Hide resolved

lefessan force-pushed the z-2025-01-03-fix-profile-evaluate branch 2 times, most recently from 276e97a to 0867bf3 Compare January 3, 2025 14:42

GitMensch reviewed Jan 3, 2025

View reviewed changes

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

lefessan added 3 commits January 7, 2025 00:03

add test for profiling EVALUATE bug

48fdda3

fix profiling bug

f684913

Fix INDEXED BY in LINKAGE SECTION

e259ff7

Fix a bug where INDEXED BY in LINKAGE SECTION would cause the variable not to be added to the local include file, causing a compile error of the C code.

lefessan force-pushed the z-2025-01-03-fix-profile-evaluate branch from 0867bf3 to e259ff7 Compare January 6, 2025 23:03

emilienlemaire added 4 commits January 23, 2025 11:22

Update tests

c18eeb1

Add profiling check

dd20fc2

Fix wrong commited files

f3fe0da

Add profiling result

f988e82

GitMensch requested changes Apr 15, 2025

View reviewed changes

		AT_SETUP([INDEXED BY in LINKAGE])

		AT_KEYWORDS([runmisc OCCURS SEARCH])

		AT_SETUP([profiling and codegen])
		AT_KEYWORDS([EVALUATE])

Conversation

lefessan commented Jan 3, 2025

Uh oh!

GitMensch commented Jan 3, 2025

Uh oh!

Uh oh!

lefessan commented Jan 3, 2025

Uh oh!

Uh oh!

lefessan commented Jan 3, 2025

Uh oh!

GitMensch commented Jan 3, 2025

Uh oh!

lefessan commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GitMensch commented Jan 3, 2025

Uh oh!

lefessan commented Jan 3, 2025

Uh oh!

GitMensch commented Jan 3, 2025

Uh oh!

GitMensch commented Jan 3, 2025

Uh oh!

lefessan commented Jan 6, 2025

Uh oh!

GitMensch commented Jan 7, 2025

Uh oh!

lefessan commented Jan 7, 2025

Uh oh!

GitMensch commented Jan 7, 2025

Uh oh!

emilienlemaire commented Jan 23, 2025

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lefessan commented Jan 3, 2025 •

edited

Loading