-
Notifications
You must be signed in to change notification settings - Fork 24
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
Move static variables to local state structures in libcob/strings.c #137
Move static variables to local state structures in libcob/strings.c #137
Conversation
Correct. That would also be a much more global change with extensive tests of actual threading needed. Postponed to later.
Sounds good. Just have seen what I've did when changing existing ABI last time: https://github.com/OCamlPro/gnucobol/blob/gcos4gnucobol-3.x/HACKING#L102 - so please have GnuCOBOL 3.1 installed on the system, then run ...now on to the actual review. |
Do you mean these variables should remain static? |
Not necessary, that's just something to watch out for. I'm nearly done with the first review (there will be another one, after your next push, but that already gives you something to work on) - overall looks good. |
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.
first review; the next iteration possibly focuses on the compat-mode and inspects for performance implications (both not inspected yet).
Thanks for your work, that is definitely going into the right direction and should be about 80% done (90% after handling the review comments)
cobc/typeck.c
Outdated
@@ -14389,6 +14401,8 @@ cb_emit_string (cb_tree items, cb_tree into, cb_tree pointer) | |||
cb_tree l; | |||
cb_tree end; | |||
cb_tree dlm; | |||
cb_tree st = cb_build_direct ("string_st", 0); | |||
cb_tree pst = cb_build_direct ("&string_st", 0); |
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.
That should likely be CB_BUILD_CAST_ADDRESS (st)
or CB_BUILD_CAST_ADDR_OF_ADDR (st)
(would need to check the result), same for the other references.
As you always need pst only once - directly replace that instead at its used place instead of defining a function-wide tree.
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 tried that:
cb_tree st = cb_build_direct ("string_st", 0);
...
CB_BUILD_CAST_ADDRESS (st);
(same with CB_BUILD_CAST_ADDR_OF_ADDR
)
but I end up with a
cobc: unexpected tree tag: DIRECT
cobc: codegen.c:1292: internal compiler error
error. I assume I should not have used cb_build_direct
in cb_tree st = cb_build_direct ("string_st", 0);
. I currently don't see how I'm supposed to build the string_st
identifier (structure pointer) otherwise. Moreover, it doesn't look like there're other similar examples with known identifiers used as arguments. I can dig more but if you have that information, I'm interested.
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.
@GitMensch Do you have an answer for this remark? I mistakenly marked it as resolved so you probably did not notice it.
bc04d87
to
37b77de
Compare
I did most fixes. However:
Hence, I had to compile COBOL programs with newer version of the compiler (>2.2), some being compiled with GnuCOBOL 3.2. |
What do you mean by this? |
This comment was marked as resolved.
This comment was marked as resolved.
I'd suggest to do the following changes:
|
|
No, that's of course |
It works. Do you have an explanation for that? |
I don't understand the question. |
c1a8cd3
to
db7bb04
Compare
I'm not sure to understand why this fix is working. Does GnuCOBOL 2.2 generate code which is not strictly correct for newer versions, meaning we need to use functions like There's also IDENTIFICATION DIVISION.
PROGRAM-ID. charset.
ENVIRONMENT DIVISION.
CONFIGURATION SECTION.
SPECIAL-NAMES.
ALPHABET ALPHA IS ASCII.
ALPHABET BETA IS EBCDIC.
DATA DIVISION.
WORKING-STORAGE SECTION.
01 TESTHEX PIC X(10) VALUE X'C17BD6F2F0F1F8406A5A'.
procedure division.
sample-main.
INSPECT testhex CONVERTING BETA TO ALPHA
DISPLAY 'Converted: "' TESTHEX '"' WITH NO ADVANCING
GOBACK.
END PROGRAM charset. I get
|
Those are inlined functions, so they are compiled back then and may conflict with newer ones, but that doesn't matter, as those are inline. Note: this happens for "optimized" functions, the "general" ones should always work (if they aren't added new). The important part are the external ones, in this case for "string" handling.
Hm, that's an old option, maybe using defined alphabets doesn't work? Please try |
bf73e39
to
b0109b9
Compare
|
I just noticed that on test
It works with >=3.1 though. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
66bcf1d
to
46ac59c
Compare
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.
That looks quite good; the memory handling (extra free functions for the state and using cob_malloc
during allocation) is likely the most important needed change.
As this PR adds a state, I think the enum type is best moved there - but that's actually optional.
Once the memory change is done, I'll check the coverage output from the CI to ensure we didn't add something untested, then this can finally go upstream.
cobc/codegen.c
Outdated
if (prog->flag_inspect_used) { | ||
output_line ("if (inspect_st != NULL)"); | ||
output_line ("\tcob_free (inspect_st);"); | ||
} | ||
if (prog->flag_string_used) { | ||
output_line ("if (string_st != NULL)"); | ||
output_line ("\tcob_free (string_st);"); | ||
} | ||
if (prog->flag_unstring_used) { | ||
output_line ("if (unstring_st != NULL)"); | ||
output_line ("\tcob_free (unstring_st);"); | ||
} |
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.
these contain allocated childs like dlm_list and repdata which need to be freed; please add the new externalized functions cob_inspect_free
, cob_string_free
, cob_unstring_free
and use them both here and in cob_exit_strings
(which should include the code for these functions) for the static variants
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.
Is there a good way to free cob_field
variables or can I just use cob_free
? I see a cob_field_free
function in libcob/reportio.c
.
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 also remark that cob_field
variables are not freed and when I try to free them, I get an invalid pointer error (I didn't investigate this yet).
tests/Makefile.am
Outdated
@@ -64,7 +64,8 @@ testsuite_sources = \ | |||
testsuite.src/data_packed.at \ | |||
testsuite.src/data_pointer.at \ | |||
testsuite.src/numeric-dump.cob \ | |||
testsuite.src/numeric-display.cob | |||
testsuite.src/numeric-display.cob \ | |||
testsuite.src/backcomp.at |
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.
please move that new entry up before the .cob files (additional benefit: only a one-line change)
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.
.. and change -2023
in the file header to -2024
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…655-4658, 4663 from branches/gnucobol-3.x: ........ build_windows: adjusted svn properties (global-ignores instead of single ones) ........ preparation for CANCEL ALL [feature-requests:#164] runtime part mostly missing, too much effort for now, compiler part finished ........ minor cleanup related to resolving COBOL modules * bin/cobcrun.c: no module name check (done in libcob now) * libcob/call.c: * (cob_resolve_internal): added check for module name length, fixed compiler warnings * call.c (cob_call): switched argument table from dynamic allocation to stack allocation fixed [bugs:#844] stack-based buffer overflow found with fuzzing cobc/tree.c (literal_for_diagnostic): fixed #844 stack-based buffer overflow ........ follow-up to [r4625] "move of defaultbyte from flag to dialect option" NEWS entry added cobc: * config.def, config.c: changed defaultbyte option from "INT" to ANY" with explicit check for "init" (GnuCOBOL default behavior, in r4625 as "ignore", now also allowing to set it after it was set different) and "none" (in preparation of missing feature, for now => implicit 0) config: * set defaultbyte to "none" for standard COBOL, 32 to " " ........ literal handling overhaul, first time working national literals cobc: * scanner.l (read_literal): do the necessary conversion for national literals (simple approach, only working with source in iso-8859-15 or plain ascii) * typeck.c (get_value): return correct numeric value for national (utf16) literals * tree.c (cb_build_intrinsic): fixed optimized length generation for national fields and literals * typeck.c (cb_validate_program_environment): refactored, reducing variable scope and extracted (validate_alphabet) and (check_class_duplicates); call the later depending on cb_warn_additional, no need to test if the final result is ignored * typeck.c (validate_alphabet): adjustments for national literals, now partially supported * scanner.l: moved static literal_error to local variable, passing it (to error_literal); return a valid literal in case of literal errors (intead of cb_error_node) to prevent spurious follow-up errors on their use ........ follow-up to [r4625] "move of defaultbyte from flag to dialect option" config: * ibm-strict.conf, mvs-strict.conf, gcos-strict.conf, bs2000-strict.conf: adjusted defaultbyte to 0 ........ Merged revisions 4548-4549,4553-4554,4624-4625 from trunk: Fix for BIT-WISE ops on Bigendian ........ added test for internal initialize for WORKING-STORAGE - revival of [bugs:#694] ........ Fix codegen issues ........ Fix for WORKING Initialize with OCCURS/VALUE ........ Improve INITIALIZE - more later ........ Move defaultbyte from flag.def to config.def ........ add preliminary support for DEFAULT SECTION cobc: * pplex.l, parser.y: parse DISPLAY and ACCEPT statements in DEFAULT SECTION (GCOS 7 extension) ........ add support for more source formats [feature-requests:#29] support for ACUCOBOL-GT Terminal format [feature-requests:#230] support for X/Open Free-form format * NEWS: added news entry for source formats * configure.ac: check for __attribute__((pure)) cobc: * cobc.h, cobc.c: extend cb_format enum with VARIABLE, TERMINAL, XOPEN, XCARD, CRT, and COBOLX source formats * cobc.c, flag.def: add new flag -fformat * ppparse.y: extend SOURCEFORMAT directive * cobc.h, cobc.c, pplex.l (cobc_get_indicator_column, cobc_get_text_column, cobc_get_indicator, cobc_get_margin_a, cobc_get_margin_b): encapsulate source format-related variables with pure functions * cobc.c: drop source format-related macros * cobc.h, pplex.l (cobc_deciph_source_format, cobc_set_source_format, cobc_get_source_format): encapsulate source format configuration into preprocessor lexer * pplex.l (ppinput): add support for ACU terminal and X/Open indicators, as well as floating margin B * pplex.l (check_listing): do not output sequence number of short lines * cobc.h: define function purity attribute COB_A_PURE * cobc.c (cobc_print_info): silence a warning with string indexing doc: * gnucobol.texi: document newly supported source formats ........ add support for CONTROL DIVISION (with SUBSTITUTION SECTION only) cobc: * pplex.l, ppparse.y: add support for CONTROL DIVISION (GCOS 7 extension); only SUBSTITUTION SECTION is handled yet * config.def: new control-division option config: * general: added option control-division ........ compiler speedup cobc/field.c.c (cb_resolve_redefines): always search candidate with (small) word list first, instead of checking the complete parent for a same name with case-insensitive name comparison ........ fixed syntax check for DISPLAY AT cobc/typeck.c (numeric_children_screen_pos_type): ignore redefined fields ........ allow DEPENDING clause in RECORD CONTAINS cobc: * config.def, parser.y: allow DEPENDING clause in RECORD CONTAINS config: * general: added option record-contains-depending-clause Co-authored-by: David Declerck <[email protected]> ........ add GCOS-specific device name mnemonics cobc: * config.def, typeck.c (cb_emit_accept_name, cb_build_display_name): rely on new option device-mnemonics (boolean) instead of standard-define to accept device name mnemonics for DISPLAY and ACCEPT * parser.y, reserved.c, scanner.l, typeck.c: add GCOS-specific mnemonics ALTERNATE-CONSOLE, ALTERNATE CONSOLE and TERMINAL config: * general: added config option device-mnemonics (boolean) Co-athored-by: David Declerck <[email protected]> ........ libcob/strings.c (cob_unstring_into): minor performance-tweak for UNSTRING with a single DELIMITED BY phrase ........ parse WITH CONVERSION clause for DISPLAY statement cobc: * parser.y: allow the WITH CONVERSION clause right after DISPLAY (ignored) Co-authored-by: David Declerck <[email protected]> ........ [feature-requests:#137] support literal operands in partial replacing phrases cobc: * ppparse.y (literal_token): support SPACE or SPACES figurative constant as second operand of partial replacing phrases * pplex.l, ppparse.y, config.h: support COPY and REPLACE statements with partial REPLACING operands specified using literals * config.def: new option partial-replacing-with-literal * cobc.h, pplex.l (ppparse_verify): feature verification while in ppparse.y * pplex.l (ppparse_error): shift newline counter by one when reporting an error when in ppparse.y config: * general: added option partial-replacing-with-literal ........ factorize some code in the compiler cobc: * ppparse.y (unquote, fix_filename): factorize code for unquotation of alphanumeric literals ........ add support for the STOP ERROR statement config: * general: add a stop-error-statement option cobc: * config.def, parser.y: add support for the STOP ERROR statement libcob: * common.c, common.h (cob_stop_error): new function for STOP ERROR statement Co-authored-by: Fabrice Le Fessant <[email protected]> ........
231f0e7
to
d5194a0
Compare
That's strange - which ones? The only one I can think of are new elements like the GCOS |
Some have already been mentioned before:
And new ones:
My own personal testing file (compiled on 3.1) has the following error:
(but works fine in GC 3.1+) |
Here is the test I use to cover strings: |
these are all no backward-compatibility points, so can be dropped here until we change the codegen
new ones: instead of for your personal test: just comment that first part out, as the codegen was not possible with 2.2 |
How I did it (same for ASCII, by commenting/commenting out the right parts): IDENTIFICATION DIVISION.
PROGRAM-ID. prog.
ENVIRONMENT DIVISION.
CONFIGURATION SECTION.
OBJECT-COMPUTER.
x86 PROGRAM COLLATING SEQUENCE IS EBCDIC-CODE.
SPECIAL-NAMES.
ALPHABET EBCDIC-CODE IS EBCDIC.
PROCEDURE DIVISION.
* IF EXPECT-ORDER = 'ASCII'
* IF "1" NOT < "a"
* ELIF EXPECT-ORDER = 'EBCDIC'
IF "a" NOT < "1"
* END-IF
DISPLAY "ERROR" END-DISPLAY
END-IF.
STOP RUN. |
That's the way to go and works fine, right? |
If works for everything except one test. It's weird but I get 'ERROR' when running tests with |
Ok, I just realized it only works for GC 3.2+ (it's the code above) |
If I understood correctly the test may better be named When generating those with cobc 2.2 you'd best use two different programs one with ASCII and one with EBCDIC collation and expectation (not only drop the ascii, as this is likely not working when running the test on an ebcdic machine later). |
The program for EBCDIC doesn't work ( |
Checked and recognized the error when adding
With GC 2.2 you should compile both tests with |
fc96fec
to
4c9e3ca
Compare
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 double use of backcomp.at in testsuite.at and the place where it is in the makefile, this PR is good to go upstream - feel free to commit with the changes directly in SVN or fix those before
tests/Makefile.am
Outdated
@@ -64,7 +64,8 @@ testsuite_sources = \ | |||
testsuite.src/data_packed.at \ | |||
testsuite.src/data_pointer.at \ | |||
testsuite.src/numeric-dump.cob \ | |||
testsuite.src/numeric-display.cob | |||
testsuite.src/numeric-display.cob \ | |||
testsuite.src/backcomp.at |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -78,6 +78,10 @@ | |||
* termio.c (pretty_display_numeric), mlio.c (get_num): | |||
changed to use the attributes of the receiving field | |||
|
|||
2024-07-19 Simon Sobisch <[email protected]> | |||
* coblocal.h (COB_TLS): add a new attribute for thread local static. |
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.
oh, here and below add an empty lime after the name
e83f7a3
to
db2b67b
Compare
6ea24d5
to
246a3a9
Compare
OK everything should be fine now 🙏 |
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.
happy commit upstream
Ah, forgot about this one, I'll take care of merging it on SVN. |
Merged on SVN @ 5302 (to reattribute to Boris). |
thanks, done |
@GitMensch I just want to thank you for all your (meticulous) reviews even though I was struggling sometimes and had a lot of catching up to do on a lot of things. This PR is not great change to GnuCOBOL but I actually learned a lot. |
Is the upstream update from gnucobol-3.x branch to gcos4gnucobol-3.x automatic or does this need to be manually triggered, or even done? |
We manually merge gnucobol-3.x into gcos4gnucobol-3.x from time to time. I'll do that in a bit. |
Answer to feature request #448 on Sourceforge.
Problem
The handling of the
INSPECT
,STRING
andUNSTRING
statements inlibcob/strings.c
is an obvious source of unthread-safe code because it relies on a lot of static variables.The idea was to:
_mt
(for "multi-thread") suffix which uses local state structures;Comments about the modifications
In each generated C program, state structures are defined as local variables and used by the functions for
INSPECT
,STRING
andUNSTRING
statements.Some "local" variables where static without any serious reason:
figurative_ptr
,figurative_size
,alpha_fld
andstr_cob_low
. Thestatic
keyword has been removed.cobglobptr
variable is still static. I am not sure how to deal with it. What I have in mind is to turn it into a function argument and pass it to function calls in the generated C code (where it is defined as a local variable in.c.l.h
files). But this probably goes outside the scope of the intial problem.Testing
I added a new test suite
tests/testsuite.src/backcomp_strings.at
which contains the C code (compiled with the compiler before my modifications) for each tests oftests/testsuite.src/run_misc.at
dealing with theINSPECT
,STRING
andUNSTRING
statements.All tests of the new test suite passes with both the version of the compiler before and after my modifications.