-
Notifications
You must be signed in to change notification settings - Fork 36
Fail on EC-SIZE exceptions as per the standard
#247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: gitside-gnucobol-3.x
Are you sure you want to change the base?
Fail on EC-SIZE exceptions as per the standard
#247
Conversation
|
Why do we need a new flag instead of -fec? Note: no changelog needed for new tests |
|
The new flag is introduced because the current behavior is to continue normally when fatal exceptions are raised and are not checked, which is not the behavior described by the standard. To avoid breaking programs running on GC3 after the PR is merged I added the flag, so now user can choose how these exceptions should behave, i.e do nothing as the default behavior or fail with the flag set. |
|
I see but that's exactly what -fec is for: giving fine grained control about which exceptions are checked. We have a known defect that -fec=size does not work in every case and that will be fixed with this PR being finished.
If people compile with -fec=all or --debug and don't want the the working check, they can just add -fno-ec=size
(But we only got multiple people reporting that this does not abort, even with those runtime checks enabled).
Emilien Lemaire ***@***.***> schrieb am Dienstag, 7. Oktober 2025 um 20:35:
… emilienlemaire left a comment (OCamlPro/gnucobol#247)
The new flag is introduced because the current behavior is to continue normally when fatal exceptions are raised and are not checked, which is not the behavior described by the standard.
To avoid breaking programs running on GC3 after the PR is merged I added the flag, so now user can choose how these exceptions should behave, i.e do nothing as the default behavior or fail with the flag set.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
|
|
I edited the tests that shouldn't work with |
-fstandard-exceptionEC-SIZE exceptions as per the standard
|
While I'll need to check the details (@ddeclerck can possibly help with the all failing CI tests):
|
|
I added the tests for all the exceptions that are set in libcob |
dc54b34 to
ae7da33
Compare
|
I also added a configuration option to enable the failure of the exceptions. I figured many could want to run with |
GitMensch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we getting near to the solution :-)
7b0b2cf to
6343a92
Compare
|
|
Hello @GitMensch, I made this commit (which will be edited), and the runtime knowledge is the last part I have to add, so I was wondering how I should add the information on the exceptions that enabled / disabled ? |
f6f1356 to
7a7b37e
Compare
|
👋 Emilien and thanks again for working on this. Concerning your quuestion - the information about the checked exceptions is codegen'd into the module - it can even be different "per line" using the TURN directives.
that approach should be visible if you now generate with Your last commit seems to do that quite cleanly, no? How does the generated code with for example a check for divzero look like? Did you do anything to the conditions ( What are the parts this PR needs a direction / review on? Note: instead of a |
7a7b37e to
5360c4b
Compare
|
Hello Simon! Thank you for the help! I updated my commits, and hopefully we are getting closer to get it merged 😄 Concerning the I think I made the changes you were looking for, yet this pose a problem: when any |
|
Why do the changes here fail one of the NC tests? did the behavior changed between standards (and we miss a possible distinction between COBOL85 and 2002+) or is there an error in the NIST test or does it depend on undefined behaviour ... or there is an error in the PR itself? Note: In all cases: please try to "extract" the failure into one of the tests in our own testsuite before working on fixing it,
Thanks. In effect it should have a "post check" generated that inspects the error code - similar as the generation without the SIZE ERROR clause. For the EC-SIZE error problem - that's no issue as long as the check either:
Just a fair warning: I'm likely not be available for any (or any longer) reviews this week (and also did not checked the current changes either, just the CI results). |
|
There was no |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gitside-gnucobol-3.x #247 +/- ##
========================================================
+ Coverage 67.34% 67.36% +0.01%
========================================================
Files 34 34
Lines 61487 61535 +48
Branches 16022 16045 +23
========================================================
+ Hits 41411 41451 +40
+ Misses 14146 14142 -4
- Partials 5930 5942 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GitMensch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just stumbled over an interesting explicit rule in COBOL2027 (before it possibly wasn't specified, would have to check): if the EC-SIZE happens on "item identification", then the ON SIZE phrase is not used but the fatal exception - if enabled should be done.
That is the part that will be covered by the "expression addition" - we should definitely add it to our testsuite - even with an expected failure:
MOVE 0 TO SOME
ADD TABLE-VAR (5 / SOME) TO ADD TABLE-VAR (5)
ON SIZE ERROR DISPLAY 'Should not be taken'.
The changes look good in general, but apart from some style and other minor issues there's one issue that is logically wrong - see the parser comment about NOT ON SIZE ERROR; I guess that some of the NIST tests don't need the -fno-ec=size flags because of that - but this can be easily checked after adjusting parser.y
GitMensch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart from the things mentioned in the PR this is good for upstream; please commit the missing adjustments and I can make it available
|
The PR should be OK to merge now, I should have edited everything you asked, and also fixed a few side effects those edits lit up, the main one being to reset the check of the exception before a new statement but still allow checking the exception with intrinsic functions (I checked on the standard, and the exception check should be reset between every statement, but the last exception code and stuff should still be available with intrinsics) |
GitMensch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Currently NIST NC fails, but I guess you'll push a fix soon.
|
Yes I'm working on a fix |
|
Something is broken. If we compile NC2032A with -fec=all (or just --debug) it aborts with The perform in 403 is 048300 DIVIDE 25COUNT INTO 100 GIVING 25ANS REMAINDER 25REM NC2034.2
048400 ON SIZE ERROR NC2034.2
048500 PERFORM PASS NC2034.2 *> here
048600 GO TO DIV-WRITE-F4-4-0. NC2034.2the ADD is 030900 PASS. MOVE "PASS " TO P-OR-F. ADD 1 TO PASS-COUNTER. NC2034.2so the Before any change in codegen it would good to have a similar example in the testsuite! The reason is that before the statements that check the exceptions after their execution we need to reset it, which is done in codegen.c by those lines: if (!p->file && (p->ex_handler || p->not_ex_handler)) {
output_line ("cob_glob_ptr->cob_exception_code = 0;");
}which are not taken because no handler is set. I suggest to add a check This should lead to much more (all?) NC tests not needing For the ec handler output I've meant something along static void
output_ec_size_handler (void)
{
int ec_checked = 0;
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_ADDRESS)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_ADDRESS);
}
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_EXPONENTIATION)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_EXPONENTIATION);
}
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_IMP)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_IMP);
}
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_OVERFLOW)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_OVERFLOW);
}
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_TRUNCATION)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_TRUNCATION);
}
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_UNDERFLOW)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_UNDERFLOW);
}
if (CB_EXCEPTION_ENABLE (COB_EC_SIZE_ZERO_DIVIDE)) {
ec_checked |= CB_EXCEPTION_CODE (COB_EC_SIZE_ZERO_DIVIDE);
}
if (ec_checked) {
output_line("if ((cob_glob_ptr->cob_exception_code & 0x%04x) == 0x%04x)",
CB_EXCEPTION_CODE (COB_EC_SIZE), CB_EXCEPTION_CODE (COB_EC_SIZE));
output_line("\t" "cob_fatal_exception (cob_glob_ptr->cob_exception_code);");
}
}... but then possibly also have |
On this, I was doing something close to this at one point, but let's say we have only if (unlikely (mpz_sgn (d2->value) == 0)) {
d1->scale = COB_DECIMAL_NAN;
cob_set_exception (COB_EC_SIZE_ZERO_DIVIDE);
return;
}then the generated condition will be true, and the exception will be wrongly raised, even if it was disabled. As for the exceptions that were wrongly raised in handler statements, the last commit should fix it 😄 |
a4a7515 to
0315f70
Compare
Of course my code was wrong, should have been something like if (ec_checked) {
output_line("if ((cob_glob_ptr->cob_exception_code & 0x%04x) == 0x%04x) && ((cob_glob_ptr->cob_exception_code & 0x%04x) != 0))",
CB_EXCEPTION_CODE (COB_EC_SIZE), CB_EXCEPTION_CODE (COB_EC_SIZE)), (ec_checked & 0x00FF);--> group is set, and one of the |= exception bits are set |
|
I implemented the check you suggested, but had to edit the exception codes in |
1bb1708 to
c552ba3
Compare
c552ba3 to
7c5752d
Compare
No description provided.