Integration of EXEC preprocessor beyond baseline. - #294
Conversation
GitMensch
left a comment
There was a problem hiding this comment.
- new changes need a new Changelog entry
- we'll need a NEWS entry for the new --preparser, as well as help.c and gnucobol.texi addition; the later should also document a bit about the file format
- please add your tests (you've seem to have done some) to used_binaries.at; we'd need at least:
- --preparser for not existing file
- --preparser for existing, but invalid file
- --preparser for existing file
- --preparser for two existing files
- --preparser, where pseudo preparser runs fine
- --preparser, where psuedo preparser runs in error
| /* Resolve preparser config filename: explicit path (contains separator | ||
| * or exists as-is) is used directly; otherwise look up | ||
| * COB_CONFIG_DIR/<name>.conf, mirroring cb_load_conf_file(). */ | ||
| static FILE * | ||
| open_preparser_conf (const char *name, char *resolved, size_t resolved_size) | ||
| { | ||
| FILE *fp; | ||
| size_t i; | ||
|
|
||
| for (i = 0; name[i] != 0 && name[i] != SLASH_CHAR; i++); | ||
|
|
||
| if (name[i] != 0 || access (name, F_OK) == 0) { | ||
| /* contains a path separator, or exists as given */ | ||
| snprintf (resolved, resolved_size, "%s", name); | ||
| } else { | ||
| /* plain name: look in COB_CONFIG_DIR/<name>.conf */ | ||
| snprintf (resolved, resolved_size, "%s%c%s.conf", | ||
| cob_config_dir, SLASH_CHAR, name); | ||
| } | ||
|
|
||
| fp = fopen (resolved, "r"); | ||
| return fp; | ||
| } | ||
|
|
||
| /* Parse a single "key: value" or "key value" line. | ||
| * Leading/trailing whitespace and comments (#) are stripped. */ | ||
| static int | ||
| parse_preparser_line (char *buff, char **key, char **val) | ||
| { | ||
| char *p, *colon; | ||
|
|
||
| /* strip comment */ | ||
| if ((p = strchr (buff, '#')) != NULL) { | ||
| *p = '\0'; | ||
| } | ||
| /* strip trailing whitespace/newline */ | ||
| for (p = buff + strlen (buff); p > buff && isspace ((unsigned char)p[-1]); p--); | ||
| *p = '\0'; | ||
| /* strip leading whitespace */ | ||
| for (p = buff; isspace ((unsigned char)*p); p++); | ||
| if (*p == '\0') { | ||
| return 1; /* blank line */ | ||
| } | ||
|
|
||
| *key = p; | ||
| if ((colon = strchr (p, ':')) != NULL) { | ||
| *colon = '\0'; | ||
| p = colon + 1; | ||
| } else { | ||
| /* space-separated: key value */ | ||
| for (; *p && !isspace ((unsigned char)*p); p++); | ||
| if (*p) { | ||
| *p++ = '\0'; | ||
| } | ||
| } | ||
| for (; isspace ((unsigned char)*p); p++); | ||
| *val = p; | ||
|
|
||
| /* trim trailing whitespace of key */ | ||
| for (p = *key + strlen (*key); p > *key && isspace ((unsigned char)p[-1]); p--); | ||
| *p = '\0'; | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| /* Load an external preparser configuration file and register it | ||
| * in cb_preparser_list. Returns 0 on success, non-zero on error. */ | ||
| int | ||
| cb_load_preparser_conf (const char *name) | ||
| { | ||
| FILE *fp; | ||
| char resolved[COB_NORMAL_BUFF]; | ||
| char buff[COB_SMALL_BUFF]; | ||
| struct cb_preparser_entry *pe; | ||
|
|
||
| fp = open_preparser_conf (name, resolved, sizeof (resolved)); | ||
| if (!fp) { | ||
| cb_error (_("preparser configuration '%s' not found"), name); | ||
| return 1; | ||
| } | ||
|
|
||
| pe = cobc_main_malloc (sizeof (struct cb_preparser_entry)); | ||
| pe->tag = NULL; | ||
| pe->command = NULL; | ||
| pe->cflags = NULL; | ||
| pe->ldflags = NULL; | ||
| pe->on_error = 1; /* default: error */ | ||
| pe->used = 0; | ||
| pe->next = NULL; | ||
|
|
||
| while (fgets (buff, sizeof (buff), fp)) { | ||
| char *key, *val; | ||
|
|
||
| if (parse_preparser_line (buff, &key, &val) != 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (strcasecmp (key, "tag") == 0) { | ||
| size_t i; | ||
| pe->tag = cobc_main_strdup (val); | ||
| for (i = 0; pe->tag[i]; i++) { | ||
| pe->tag[i] = (char)toupper ((unsigned char)pe->tag[i]); | ||
| } | ||
| } else if (strcasecmp (key, "command") == 0) { | ||
| pe->command = cobc_main_strdup (val); | ||
| } else if (strcasecmp (key, "cflags") == 0) { | ||
| pe->cflags = cobc_main_strdup (val); | ||
| } else if (strcasecmp (key, "ldflags") == 0) { | ||
| pe->ldflags = cobc_main_strdup (val); | ||
| } else if (strcasecmp (key, "on-error") == 0) { | ||
| pe->on_error = (strcasecmp (val, "warn") == 0) ? 0 : 1; | ||
| } else { | ||
| cb_warning (cb_warn_unsupported, | ||
| _("unknown preparser configuration key '%s' in '%s'"), | ||
| key, resolved); | ||
| } | ||
| } | ||
| fclose (fp); | ||
|
|
||
| if (!pe->tag || !pe->command) { | ||
| cb_error (_("preparser configuration '%s' is missing 'tag' or 'command'"), | ||
| resolved); | ||
| return 1; | ||
| } | ||
|
|
||
| pe->next = cb_preparser_list; | ||
| cb_preparser_list = pe; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
those functions should be moved to config.c - and, where possible, the existing functions be refactored to not have duplicated code for how the lines are read/split
There was a problem hiding this comment.
I kept the implementation for the two differently as they follow different kinds of implementation ( config table and Linked list ) the coupling between these two may make it difficult to maintain in my opinion.
There was a problem hiding this comment.
parse_preparser_line() should be replaced by re-using the code for the config lines; to do so move the code of cb_config_entry for the parsing out of the function (the new static function gets a pointer to name+value, just as your version) - in this new variant name and value will be parsed first before cb_config_entry() checks the name (which is now intermixed).
This way we have a single new function called for both compiler and preparser config, ensuring the format is exactly identical, by keeping the way it is looked up / used out of that code.
I agree that cb_load_preparser_conf() itself should be kept separate from the config file loading (though they will look very similar) because of the struct / array approach (which we don't need fo the preparser).
| User-defined dialect configuration. | ||
|
|
||
| @item --preparser=<file> | ||
| Register external preparser configuration. |
There was a problem hiding this comment.
That's a start, but similar to the note below you need to explain what this is used for and either document the structure here or add an example definition (for esqlOC and/or GixSQL) showing the format with explanations.
| /* Resolve preparser config filename: explicit path (contains separator | ||
| * or exists as-is) is used directly; otherwise look up | ||
| * COB_CONFIG_DIR/<name>.conf, mirroring cb_load_conf_file(). */ | ||
| static FILE * | ||
| open_preparser_conf (const char *name, char *resolved, size_t resolved_size) | ||
| { | ||
| FILE *fp; | ||
| size_t i; | ||
|
|
||
| for (i = 0; name[i] != 0 && name[i] != SLASH_CHAR; i++); | ||
|
|
||
| if (name[i] != 0 || access (name, F_OK) == 0) { | ||
| /* contains a path separator, or exists as given */ | ||
| snprintf (resolved, resolved_size, "%s", name); | ||
| } else { | ||
| /* plain name: look in COB_CONFIG_DIR/<name>.conf */ | ||
| snprintf (resolved, resolved_size, "%s%c%s.conf", | ||
| cob_config_dir, SLASH_CHAR, name); | ||
| } | ||
|
|
||
| fp = fopen (resolved, "r"); | ||
| return fp; | ||
| } | ||
|
|
||
| /* Parse a single "key: value" or "key value" line. | ||
| * Leading/trailing whitespace and comments (#) are stripped. */ | ||
| static int | ||
| parse_preparser_line (char *buff, char **key, char **val) | ||
| { | ||
| char *p, *colon; | ||
|
|
||
| /* strip comment */ | ||
| if ((p = strchr (buff, '#')) != NULL) { | ||
| *p = '\0'; | ||
| } | ||
| /* strip trailing whitespace/newline */ | ||
| for (p = buff + strlen (buff); p > buff && isspace ((unsigned char)p[-1]); p--); | ||
| *p = '\0'; | ||
| /* strip leading whitespace */ | ||
| for (p = buff; isspace ((unsigned char)*p); p++); | ||
| if (*p == '\0') { | ||
| return 1; /* blank line */ | ||
| } | ||
|
|
||
| *key = p; | ||
| if ((colon = strchr (p, ':')) != NULL) { | ||
| *colon = '\0'; | ||
| p = colon + 1; | ||
| } else { | ||
| /* space-separated: key value */ | ||
| for (; *p && !isspace ((unsigned char)*p); p++); | ||
| if (*p) { | ||
| *p++ = '\0'; | ||
| } | ||
| } | ||
| for (; isspace ((unsigned char)*p); p++); | ||
| *val = p; | ||
|
|
||
| /* trim trailing whitespace of key */ | ||
| for (p = *key + strlen (*key); p > *key && isspace ((unsigned char)p[-1]); p--); | ||
| *p = '\0'; | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| /* Load an external preparser configuration file and register it | ||
| * in cb_preparser_list. Returns 0 on success, non-zero on error. */ | ||
| int | ||
| cb_load_preparser_conf (const char *name) | ||
| { | ||
| FILE *fp; | ||
| char resolved[COB_NORMAL_BUFF]; | ||
| char buff[COB_SMALL_BUFF]; | ||
| struct cb_preparser_entry *pe; | ||
|
|
||
| fp = open_preparser_conf (name, resolved, sizeof (resolved)); | ||
| if (!fp) { | ||
| cb_error (_("preparser configuration '%s' not found"), name); | ||
| return 1; | ||
| } | ||
|
|
||
| pe = cobc_main_malloc (sizeof (struct cb_preparser_entry)); | ||
| pe->tag = NULL; | ||
| pe->command = NULL; | ||
| pe->cflags = NULL; | ||
| pe->ldflags = NULL; | ||
| pe->on_error = 1; /* default: error */ | ||
| pe->used = 0; | ||
| pe->next = NULL; | ||
|
|
||
| while (fgets (buff, sizeof (buff), fp)) { | ||
| char *key, *val; | ||
|
|
||
| if (parse_preparser_line (buff, &key, &val) != 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (strcasecmp (key, "tag") == 0) { | ||
| size_t i; | ||
| pe->tag = cobc_main_strdup (val); | ||
| for (i = 0; pe->tag[i]; i++) { | ||
| pe->tag[i] = (char)toupper ((unsigned char)pe->tag[i]); | ||
| } | ||
| } else if (strcasecmp (key, "command") == 0) { | ||
| pe->command = cobc_main_strdup (val); | ||
| } else if (strcasecmp (key, "cflags") == 0) { | ||
| pe->cflags = cobc_main_strdup (val); | ||
| } else if (strcasecmp (key, "ldflags") == 0) { | ||
| pe->ldflags = cobc_main_strdup (val); | ||
| } else if (strcasecmp (key, "on-error") == 0) { | ||
| pe->on_error = (strcasecmp (val, "warn") == 0) ? 0 : 1; | ||
| } else { | ||
| cb_warning (cb_warn_unsupported, | ||
| _("unknown preparser configuration key '%s' in '%s'"), | ||
| key, resolved); | ||
| } | ||
| } | ||
| fclose (fp); | ||
|
|
||
| if (!pe->tag || !pe->command) { | ||
| cb_error (_("preparser configuration '%s' is missing 'tag' or 'command'"), | ||
| resolved); | ||
| return 1; | ||
| } | ||
|
|
||
| pe->next = cb_preparser_list; | ||
| cb_preparser_list = pe; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
parse_preparser_line() should be replaced by re-using the code for the config lines; to do so move the code of cb_config_entry for the parsing out of the function (the new static function gets a pointer to name+value, just as your version) - in this new variant name and value will be parsed first before cb_config_entry() checks the name (which is now intermixed).
This way we have a single new function called for both compiler and preparser config, ensuring the format is exactly identical, by keeping the way it is looked up / used out of that code.
I agree that cb_load_preparser_conf() itself should be kept separate from the config file loading (though they will look very similar) because of the struct / array approach (which we don't need fo the preparser).
There was a problem hiding this comment.
you want one entry for the baseline (with the old date) and another one for the follow-up work, which then outlines the changes in config.c, tree.h, ...
There was a problem hiding this comment.
that whole block should - for the baseline changes - be after the may changes, nomt up-front
| #include <string.h> | ||
| #include <limits.h> | ||
|
|
||
| #include <ctype.h> |
There was a problem hiding this comment.
the existing code has no isspace, so you can get rid of that header when refactoring the existing code to be used for the tag/value split
| 2026-07-31 Uttam Bhadauriya <uttamsinghbhadoriya23@gmail.com> | ||
|
|
||
| * cobc/cobc.h, cobc/tree.h, cobc/cobc.c, cobc/config.c, cobc/pplex.l: | ||
| Refactor external preparser subsystem, move struct and functions, | ||
| rename tag to subsystem, improve error messages and indentation. | ||
| * cobc/help.c, doc/gnucobol.texi: Add --preparser documentation. | ||
| * tests/testsuite.src/used_binaries.at: Add preparser tests. | ||
|
|
There was a problem hiding this comment.
cobc and doc go to their own changelog files, the testsuite sources don't get an entry for new elements
|
|
||
| * New GnuCOBOL features | ||
|
|
||
| ** cobc provides a --preparser option to register external preparser configurations |
There was a problem hiding this comment.
explain here that EXEC is now supported using external preparsers, registered with --preparser (you can add a hint to check the details in the manual, similar to the existing entries doing so) with a fallback of ignoring any EXEC parts but INCLUDE
| * New GnuCOBOL features | ||
|
|
||
| ** cobc provides a --preparser option to register external preparser configurations | ||
|
|
There was a problem hiding this comment.
new entries to the NEWS file are added at the end of the relevant chapter
| AT_CHECK([$COBC --preparser missing.conf prog.cob], [1], [], [stderr]) | ||
| AT_CHECK([$GREP "cannot load preparser configuration 'missing.conf'" stderr], [0], [ignore], [ignore]) |
There was a problem hiding this comment.
haven't checked the ones below, but instead of placing that to an stderr where you grep from (which either needs a redirection or I learn something new here) you'd just check the output in that [] directly.
| AT_CLEANUP | ||
|
|
||
| AT_SETUP([external preparser --preparser]) | ||
| AT_KEYWORDS([preparser]) |
There was a problem hiding this comment.
| AT_KEYWORDS([preparser]) | |
| AT_KEYWORDS([cobc configuration EXEC]) |
you can replace the keyword by cobc + configuration - other words are already included by AT_SETUP content
|
@utam-1 please rebase, which should improve the MSVC part and update per review, so we can have a next look at that |
MacOS: pass std=gnu17 MSYS2: disable screenio tests for now overall actions update (fix npm deprecation)
cobc/:
* config.def: "tab-width" option is changed to a comma-separated
list of tab widths, for example "6,1,4", the last one being reused.
This new meaning is backward compatible.
Implements FR #498 " Adjust tab-width to optionally be a list of
tab-stop positions"
* config.c: initialize cb_tab_width as a string,
each position indicating the number of spaces to insert for a tab
at that position
* pplex.c,cobc.c: use the new type of cb_tab_width
cobc: * tree.h, parser.y: change type of cobc_cs_check flags to permit distinguishing more grammar contexts * cobc.h, parser.y, reserved.c (lookup_reserved_word): generalize and cleanup device for handling special contexts * parser.y: use new macros __CS_ENSURE, __CS_CLEAR, __CS_CLEAR_ALL, __CS_CHECK, __CS_LEAVE, __CS_ENTER, to help debug and explicitly manipulate special contexts * error.c (cb_error_always, cb_error_internal), tree.h, tree.c (cb_build_program), scanner.l: replace cobc_in_repository flag with a check on a special context * scanner.l: introduce cobc_in_exit_statement helper macro
6581475 to
4054ab6
Compare
I have pushed my changes, and apologies for the delay I was a bit occupied. |
cobc/:
* config.def: "tab-width" option is changed to a comma-separated
list of tab widths, for example "6,1,4", the last one being reused.
This new meaning is backward compatible.
Implements FR #498 " Adjust tab-width to optionally be a list of
tab-stop positions"
* config.c: initialize cb_tab_width as a string,
each position indicating the number of spaces to insert for a tab
at that position
* pplex.c,cobc.c: use the new type of cb_tab_width
cobc: * tree.h, parser.y: change type of cobc_cs_check flags to permit distinguishing more grammar contexts * cobc.h, parser.y, reserved.c (lookup_reserved_word): generalize and cleanup device for handling special contexts * parser.y: use new macros __CS_ENSURE, __CS_CLEAR, __CS_CLEAR_ALL, __CS_CHECK, __CS_LEAVE, __CS_ENTER, to help debug and explicitly manipulate special contexts * error.c (cb_error_always, cb_error_internal), tree.h, tree.c (cb_build_program), scanner.l: replace cobc_in_repository flag with a check on a special context * scanner.l: introduce cobc_in_exit_statement helper macro
Credits goes to Saurabh Kumar for the use of vswhere to locate the Visual Studio installation. Co-authored-by: Saurabh Kumar <developer.saurabh@outlook.com>
c9fe3f3 to
7b34538
Compare
|
@GitMensch I think the issue with msvc persists, maybe I made some mistake while doing the rebase. I'm not sure though. |
| # 1. nonexistent file -> error | ||
| AT_CHECK([$COBC --preparser ./missing.conf prog.cob], [1], [], | ||
| [configuration error: | ||
| ./missing.conf: No such file or directory | ||
| ]) | ||
|
|
||
| # 2. invalid file -> error | ||
| AT_DATA([invalid.conf], [ | ||
| # missing subsystem and command | ||
| cflags: -O2 | ||
| ]) | ||
| AT_CHECK([$COBC --preparser ./invalid.conf prog.cob], [1], [], | ||
| [error: preparser configuration './invalid.conf' is missing 'subsystem' or 'command' | ||
| ]) |
There was a problem hiding this comment.
I suggest to move these parts to configuration.at and add the missing "unknown configuration tag" code path
| /* Simplified: Always append .conf for plain names in config dir */ | ||
| snprintf (resolved, resolved_size, "%s%c%s.conf", | ||
| cob_config_dir, SLASH_CHAR, name); |
There was a problem hiding this comment.
this simplification is likely the reason for the win32 failures:
- you want to only do the fopen below if the access worked
- the snprintf should only be done if the access worked as you want to point to the "normal" name if there is neither one with the original name nor with the config dir
There was a problem hiding this comment.
I made this change but it seems to fail again for some reason.
There was a problem hiding this comment.
you still set resolved_size if the path did not exist and use it afterwards, I'll do an edit you can inspect and we'll see if that helps
caafec2 to
c7aa539
Compare
|
@GitMensch The tests are still failing, I'm not sure what's causing this from the error messages it says about some dialect of C89 / C11 not being met ( for declartion of for-loop ) and some other path related issues are present. |
That's and it is nice to see that the message got much better over the years :-) It means "for" is a statement, and "size_t j" is a declaration. In C89 (the compatibility check done here) declarations need to be at the start of a block, so that rule is broken. The fix here is to move the size_t declaration before the for statement - but we don't need that in any case as we can directly adjust the pointer buff. |
This is rather interesting; I'll review (and learn) from the commit for both the fix related to MSVC build and this issue once it is applied. |
|
work will still take some time, so I did an untested (not even compiled so far) commit - if it fails it should still show the idea... edit: seems you only need to update the expected test result to let it pass; |
| functions to config.c; share line-parsing with cb_config_entry; | ||
| fix indentation; fix cmd_len calculation | ||
| * help.c: add --preparser option documentation | ||
| 2026-06-08 Nicolas Berthier <nicolas.berthier@ocamlpro.com> |
There was a problem hiding this comment.
| 2026-06-08 Nicolas Berthier <nicolas.berthier@ocamlpro.com> | |
| 2026-06-08 Nicolas Berthier <nicolas.berthier@ocamlpro.com> |
| * config.c: initialize cb_tab_width as a string, | ||
| each position indicating the number of spaces to insert for a tab | ||
| at that position | ||
| * pplex.c,cobc.c: use the new type of cb_tab_width |
There was a problem hiding this comment.
| * pplex.c,cobc.c: use the new type of cb_tab_width | |
| * pplex.l, cobc.c: use the new type of cb_tab_width |
... but I'm a but confused where that entry comes from...
eaa9ed4 to
37fdedc
Compare
@GitMensch I was able to get an insight on the approach, especially for the fix related to movement of pointer in |
6f664b0 to
c3c0a87
Compare
GitMensch
left a comment
There was a problem hiding this comment.
The overall tests and code look good - have you already run it sucessfully with the gixsql examples running gixsql?
If yes, please add the sample configuration for this (and possibly others like esqloc and esql) to the config folder.
If not: is there anything missing but the testing?
There are 3 main issues where I've asked (or suggested) for changes, I hope they are explained well enough - if not, feel free to ask here directly.
| * New GnuCOBOL features | ||
|
|
||
| ** cobc provides a --preparser option to register external preparser configurations | ||
|
|
| prog.cob:6: error: EXEC SQL statement ignored | ||
| ]) | ||
|
|
||
| AT_CLEANUP |
There was a problem hiding this comment.
| AT_CLEANUP | |
| AT_CLEANUP | |
don't ask me why but some diff tools work better if there's an empty line after the cleanup...
| AT_CHECK([$COMPILE_ONLY -Werror=unsupported -ffold-copy=LOWER prog.cob], [1], [], | ||
| [prog.cob:8: error: EXEC SQL INCLUDE handled as COPY | ||
| ]) |
There was a problem hiding this comment.
| AT_CHECK([$COMPILE_ONLY -Werror=unsupported -ffold-copy=LOWER prog.cob], [1], [], | |
| [prog.cob:8: error: EXEC SQL INCLUDE handled as COPY | |
| ]) | |
| AT_CHECK([$COMPILE_ONLY -Wno-unsupported -ffold-copy=LOWER prog.cob], [0], [], []) |
we want to verify that the copy is read, so no reason for an error
| AT_CHECK([$COMPILE_ONLY -Werror=unsupported -ffold-copy=LOWER prog.cob], [1], [], | ||
| [prog.cob:8: error: EXEC SQL INCLUDE handled as COPY | ||
| ]) | ||
|
|
There was a problem hiding this comment.
| AT_CHECK([$COMPILE_ONLY -Werror=unsupported -ffold-copy=LOWER prog.cob], [1], [], | |
| [prog.cob:8: error: EXEC SQL INCLUDE handled as COPY | |
| ]) | |
| AT_CHECK([$COMPILE_ONLY -Wno-unsupported -ffold-copy=LOWER prog.cob], [0], [], []) | |
same as above
| AT_CHECK([$COMPILE_ONLY -Werror=unsupported prog.cob], [1], [], | ||
| [prog.cob:9: error: missing END-EXEC at end of file | ||
| prog.cob:9: error: EXEC SQL statement ignored | ||
| ]) |
There was a problem hiding this comment.
| AT_CHECK([$COMPILE_ONLY -Werror=unsupported prog.cob], [1], [], | |
| [prog.cob:9: error: missing END-EXEC at end of file | |
| prog.cob:9: error: EXEC SQL statement ignored | |
| ]) | |
| AT_CHECK([$COMPILE_ONLY -Wunsupported prog.cob], [1], [], | |
| [prog.cob:9: error: missing END-EXEC at end of file | |
| prog.cob:9: warning: EXEC SQL statement ignored | |
| ]) |
no need to error that explicit - but the missing part should be an error in any case
| cmd_len = strlen (cb_active_preparser->command) + strlen (fn->source) + strlen (fn->preprocess) + 3; | ||
| cmd = cobc_malloc (cmd_len); |
There was a problem hiding this comment.
move those two line directly to the declaration (making cmd_len a const and reorder it)
| char *cmd; | ||
| size_t cmd_len; | ||
| int ret_sys; | ||
| const char *new_preprocess; |
There was a problem hiding this comment.
drop that var, assigning it directly to fn->preprocess
| cb_source_file = fn->source; | ||
| if (!cb_active_preparser->warn_only) { | ||
| cobc_err_exit (_("external preparser '%s' failed with exit status %d"), cb_active_preparser->subsystem, ret_sys); | ||
| } else { |
There was a problem hiding this comment.
no need for the else branch, the err_exit stops the program so everything in an else can be directly placed in the code after the closing }
| fn->source = orig_source; /* Restore the original file names */ | ||
| fn->preprocess = orig_preprocess; |
There was a problem hiding this comment.
Not checked, just wondering: have we overridden those vars at all? Aren't they still fine?
| /*This prevents collision in file paths based on manual tests*/ | ||
| new_preprocess = file_replace_extension ((char *)fn->preprocess, ".i2"); | ||
|
|
||
| fn->source = cobc_strdup (fn->preprocess); /* sqlpp.sh's output */ | ||
| fn->preprocess = new_preprocess; /* fresh file for pass 2 */ | ||
|
|
||
| if (cb_active_preparser->cflags) { | ||
| COBC_ADD_STR (cobc_cflags, " ", cb_active_preparser->cflags, NULL); | ||
| } | ||
| if (cb_active_preparser->ldflags) { | ||
| COBC_ADD_STR (cobc_ldflags, " ", cb_active_preparser->ldflags, NULL); | ||
| } |
There was a problem hiding this comment.
this can be the else branch for the failure above... actually better check if (ret_sys )= 0) { and have that in the first branch, the hopefully less likely error case in the else - and finally the goto outside of both
| e = buff + strlen (buff); | ||
| while (e > buff && (e[-1] == '\r' || e[-1] == '\n')) { | ||
| *--e = 0; | ||
| } |
There was a problem hiding this comment.
That's slightly confusing - Is there a reason to not subtract 1 when computing e and checking e directly instead of e[-1}?
You can then also just do e--; in the loop and end with a final *(e + 1) = 0; before the return.
... just wondering.
There was a problem hiding this comment.
My first thoughts were to use something cleaner and readable as you suggested, but from what I gathered the subtract 1 rule results in an undefined behaviour ( when buff is empty ) as that'll make e point to an address before the start of array. Modern architectures do not take this thing into account but maybe for stricter architectures it may result in issues.
Be as it may, I have updated the code, as per your suggestions.
There was a problem hiding this comment.
from what I gathered the subtract 1 rule results in an undefined behaviour ( when
buffis empty )
Just interested to follow your thought: How can that be with the previous code (skipping empty lines, and the separator not found)?
There was a problem hiding this comment.
You're right the previous lines do ensure that buff doesn't stay empty, but then again there are issues with aggressive compiler optimization if we're using Undefined Behaviour code, as they're picked up as warnings of some sort in old school systems, apart from that if someone includes a static analysis tools do tend to flag such codes if they're present. I personally would not vouch for encountering issues with compiler optimization when using such kind of code, however I have witnessed issues with static analysis tools complaining if we're moving away from pre-defined standards.
This is nitpicking in my humble opinion, the code as such right now wouldn't create any issues.
efacb58 to
8a9ae31
Compare
@GitMensch I have performed a crude test with GixSQL, but I believe since it wasn't rigorous enough I didn't added the config file. I am describing what I have found below: I have used the following command to invoke cobc --preparser : And from this the output I got was : I think the translation works, however, if possible, I think it would be better if you would perform more rigorous testing from your end. Apart from the testing, I don't recall there is anything else, as we have implemented almost everything based on the plan we had outlined. |
GitMensch
left a comment
There was a problem hiding this comment.
please attach your LLM findings about nested preprocessor issues to this PR
Have you tried it locally (for example with a pseudo EXEC CICS which just replaces CICS by SQL and your GixSQL test configuration)?
| /* These options were all processed in the first getopt-run */ | ||
| break; |
There was a problem hiding this comment.
| /* These options were all processed in the first getopt-run */ | |
| break; |
| /* strip trailing CR/LF for error message */ | ||
| e = buff + strlen (buff) - 1; | ||
| while (e >= buff && (*e == '\r' || *e == '\n')) { | ||
| e--; | ||
| } | ||
| *(e + 1) = 0; |
There was a problem hiding this comment.
interesting side question: what happens if the file has no trailing CR/LF and we get here?
There was a problem hiding this comment.
That wouldn't be in an issue I think, the loop would exit and the null terminator would be overwritten. The string wouldn't change.
| line++; | ||
| split_ret = cb_conf_split_line (buff, &tag_name, &tag_val); | ||
| if (split_ret == 1) { | ||
| continue; |
There was a problem hiding this comment.
| continue; | |
| continue; /* blank or comment line */ |
| configuration_error (name, line, 1, | ||
| _("invalid configuration tag '%s'"), buff); | ||
| status = 1; | ||
| continue; |
There was a problem hiding this comment.
according to the coverage check, this one misses a test case
| AT_DATA([unknown.conf], [ | ||
| subsystem: SQL | ||
| command: /bin/true | ||
| badkey: something | ||
| ]) | ||
|
|
||
| AT_DATA([prog.cob], [ | ||
| IDENTIFICATION DIVISION. | ||
| PROGRAM-ID. prog. | ||
| PROCEDURE DIVISION. | ||
| STOP RUN. | ||
| ]) | ||
|
|
||
| AT_CHECK([$COBC --preparser ./unknown.conf prog.cob], [1], [], | ||
| [configuration error: | ||
| ./unknown.conf:4: unknown configuration tag 'badkey' | ||
| ]) |
There was a problem hiding this comment.
| AT_DATA([unknown.conf], [ | |
| subsystem: SQL | |
| command: /bin/true | |
| badkey: something | |
| ]) | |
| AT_DATA([prog.cob], [ | |
| IDENTIFICATION DIVISION. | |
| PROGRAM-ID. prog. | |
| PROCEDURE DIVISION. | |
| STOP RUN. | |
| ]) | |
| AT_CHECK([$COBC --preparser ./unknown.conf prog.cob], [1], [], | |
| [configuration error: | |
| ./unknown.conf:4: unknown configuration tag 'badkey' | |
| ]) | |
| AT_DATA([unknown.conf], [ | |
| subsystem: SQL | |
| command: /bin/true # value does not matter, we check for the key below | |
| # (and here for inline parsing of comments) | |
| badkey: something | |
| ]) | |
| AT_DATA([prog.cob], [ | |
| IDENTIFICATION DIVISION. | |
| PROGRAM-ID. prog. | |
| PROCEDURE DIVISION. | |
| STOP RUN. | |
| ]) | |
| AT_CHECK([$COBC --preparser ./unknown.conf prog.cob], [1], [], | |
| [configuration error: | |
| ./unknown.conf:6: unknown configuration tag 'badkey' | |
| ]) |
| const size_t cmd_len = strlen (cb_active_preparser->command) + strlen (fn->source) + strlen (fn->preprocess) + 3; | ||
| char *cmd = cobc_malloc (cmd_len); | ||
| snprintf (cmd, cmd_len, "%s %s %s", cb_active_preparser->command, fn->source, fn->preprocess); | ||
|
|
||
| ret_sys = call_system (cmd); | ||
| cobc_free (cmd); |
There was a problem hiding this comment.
I do wonder if/how we should pass include paths explicit given to cobc, maybe just as a third long argument that contains -I path1 -I /path/two?
That still allows to do "something else" in the preparser config if -I is not supported by the preparser.
| #define YYSTYPE cb_tree | ||
| #define _PARSER_H /* work around bad Windows SDK header */ | ||
| #include "parser.h" | ||
| #include <stdio.h> |
There was a problem hiding this comment.
| #include <stdio.h> |
We don't need that as there is no new code, do we?
| #include "cobc.h" | ||
| #include "tree.h" | ||
| #include "ppparse.h" | ||
| #include <stdio.h> |
There was a problem hiding this comment.
What do we need this for?
| Note that @code{INCLUDE} statements (e.g., @code{EXEC SQL INCLUDE ...}) are | ||
| always handled by the internal preprocessor as a standard @code{COPY} statement, | ||
| with full support for library paths (@option{-I}) and case folding, before any | ||
| external preparser is considered. |
There was a problem hiding this comment.
I think that does only apply if no preparser is found:
If a preparser is configured correctly then we would get EXEC (EXEC_STATE) WORD + SUBSYSTEM_FOUND = YYACCEPT + restart --> then the configured EXEC preparser would already see and replace INCLUDE, so COBOL would never see that in the second process, no?
9780605 to
fa073ec
Compare
@GitMensch I have added a folder called |
f0b8c93 to
0058c20
Compare
Description of PR
This is a follow - up to #279.
Approach and basic idea
cb_preparser_entryis added tocobc.c, which handles the associated pre-parser metadata.cb_preparser_listandcb_active_preparserare created, one being a list to registered preparsers, and other is a pointer forpplex.lto identify the tag.cb_load_preparser_conf()andcb_find_preparser()are created based on similar mechanism ofcb_config_entry().In
ppparse.ya tokenSUBSYSTEM_FOUNDis declared which is used for identifying EOF, and for gracefully exiting the parsing.So far only general tests have been performed by using a
test.conffile and setting up a pseudo - preprocessor via bash, which shows the replacement works and flag-injection is performed via gcc.