Fixes for C23 #229

ddeclerck · 2025-05-12T15:34:04Z

This PR attempts to fix GnuCOBOL for C23, by adding appropriate function type casts, both in libcob and codegen.

There's still one test failing, due to multiple definition of the same function prototype (I guess this was working before C23 because the first prototype was incomplete ; now it includes all the arguments). Investigation led me to line 1908 in codegen.c:output_call_cache:

			output_local ("#ifndef %s\n", static_call->call_name);

Apparently the intent was to use an #ifndef to prevent multiple definition of the same symbol... yet preprocessor directives may not be used to test whether a function is defined...

ddeclerck · 2025-05-13T09:09:10Z

In order to fix the last failing test, I was able to skip generating the "default" prototype when an actual prototype is provided in the COBOL source. However, the test fails later, when testing the >>IMP directive: since this may add function prototypes, they may clash with the ones inferred for static calls, and we have no way to detect that...

GitMensch

I'm unsure how the code in call.c should work, even more if we don't use the deprecated and in C2x forbidden variable arguments option.

It seems that the best option is to pass only the amount of arguments in a way we can query from the loaded program, with an exported symbol in the shared object... but even then we would need for example libffi (we could include the sources and statically link that per default as GCC does) to do a dynamic call [and if we don't get the number of arguments from the callee, we could use the argc).

... I feel a bit stuck here (still left some comments).

Thoughts?

GitMensch · 2025-05-13T11:43:47Z

cobc/codegen.c

 	sclp->convention = convention;
 	sclp->return_type = return_type;
+	sclp->args = args;
 	sclp->next = static_call_cache;


Not checked, mostly wondering: Should the above code in the loop do anything with the args or other parameters? (an hour later) We should have all of that already done in the compiler when we build and compare the internal prototype - which should be what we use here.
Thinking of it... the internal prototype should have convention, return_type and arg list within a structure, maybe that exists and can be used here, maybe it should be added...

@ddeclerck Can you check that, please?
If we can pass the prototype instead of its components "return type" + "args", we should do so.

ping @ddeclerck - also for the other review comments and cobc/Changelog

pong ;)
I wrote that a while ago and that seemed to make sense back then.
I'd need some time to dive back into this - but with the current failure on test 27, I don't even know how much this is going to change.

GitMensch · 2025-05-13T11:46:20Z

cobc/codegen.c

 	}
 	if (static_call_cache) {
 		const char			*convention_modifier;
+		FILE				*savetarget = output_target;


Zhe code below uses output_local only and that goes to cb_local_file - What did I miss?

Below we call output_function_arg_types, which may output either to the "local" file (when outputting function definitions) or to the "main" file (when outputing casts in actual code).

Please add a comment on line 1898 that we switch that for the output_function_arg_types call.

GitMensch · 2025-05-13T13:17:28Z

cobc/codegen.c

+output_function_cast (struct cb_call *p, size_t ret_ptr, const char *convention, const char *name_str)
+{
+	output_prefix ();
+	if (ret_ptr) {


ret_ptr should never have been a size_t, please fix that here and in the callers

GitMensch · 2025-05-13T13:23:07Z

cobc/codegen.c

+static void
+output_function_cast (struct cb_call *p, size_t ret_ptr, const char *convention, const char *name_str)


this new (partially extracted) function should definitely get a good doxygen-style comment

GitMensch · 2025-05-13T15:08:14Z

libcob/call.c

 #pragma warning(suppress: 4113) /* funcint is a generic function prototype */
 	cancel_func = p->module->module_cancel.funcint;
 #else


that pragma should go away and still work fine with the new code

GitMensch · 2025-05-13T15:12:47Z

libcob/call.c

 				(void)cancel_func (-1, NULL, NULL, NULL,
 						   NULL);


I have no clue why the cancel_func is setup to have 4 NULL, especially if we have a program with no USING then the cancel_func has only a single parameter const int entry, no?
Telling C explicit how many arguments to handle and then ask it to handle too much seems to ask for trouble...

ddeclerck · 2025-05-13T16:42:08Z

I'm unsure how the code in call.c should work, even more if we don't use the deprecated and in C2x forbidden variable arguments option.

It seems that the best option is to pass only the amount of arguments in a way we can query from the loaded program, with an exported symbol in the shared object... but even then we would need for example libffi (we could include the sources and statically link that per default as GCC does) to do a dynamic call [and if we don't get the number of arguments from the callee, we could use the argc).

... I feel a bit stuck here (still left some comments).

Thoughts?

Well I'm missing the "big picture" about how GnuCOBOL handles calling in general, so I might not see the implications, but to me, I don't see much differences between:

calling without casting a function declared without a prototype
and
calling with casting a function declared with a prototype
provided that, in both cases, the actual number and types of arguments passed match the actual number and types of arguments expected by the function (otherwise we're screwed because this is undefined behavior... so the two methods are similarly unsafe).

So when we get to call a function whose prototype we don't know, let's just declare it with a dummy type, and cast it at call site according to the arguments that are passed (and in fact that's more or less what's done in this PR).

But really, users should define proper prototypes...

GitMensch · 2025-05-13T17:09:14Z

cast it at call site according to the arguments that are passed

That would be the wrong approach. It should be called according to the prototype. Note that cobc deduces the prototype when it processes a program during compilation and if it cannot deduce anything by that, then it still can deduce an internal prototype by the actual CALL statement(s).
I'm not sure how far into codegen this goes, but in general "it is there".

Agreed that the cob_call() part - which is "hairy" in any case can be changed to the new version (per gentoo patch and this PR), because that's "just a possibly different undefined behaviour than before".
For the parts that use the cancel func - please adjust it to always use a single const int - currently it is different in call.c, common.c (and potentially others), as well as adjusting the ret_ptr stuff ... and adding Changelog entries.

The codegen part is something I need to inspect again (did so longer today) - maybe you can have a look how that aligns to the internal prototypes that are for example used in the compile-time checks?

What does the COB_SYSTEM_GEN change do exactly?

ddeclerck · 2025-05-13T17:35:10Z

cast it at call site according to the arguments that are passed

That would be the wrong approach. It should be called according to the prototype.

Of course I was only suggesting this for those functions where we don't have any prototype nor we can not infer any.

Note that cobc deduces the prototype when it processes a program during compilation and if it cannot deduce anything by that, then it still can deduce an internal prototype by the actual CALL statement(s). I'm not sure how far into codegen this goes, but in general "it is there".

Agreed that the cob_call() part - which is "hairy" in any case can be changed to the new version (per gentoo patch and this PR), because that's "just a possibly different undefined behaviour than before". For the parts that use the cancel func - please adjust it to always use a single const int - currently it is different in call.c, common.c (and potentially others), as well as adjusting the ret_ptr stuff ... and adding Changelog entries.

Will do.

The codegen part is something I need to inspect again (did so longer today) - maybe you can have a look how that aligns to the internal prototypes that are for example used in the compile-time checks?

Codegen remains tricky, I'm really not confident about this code yet - and I still have one failing test. I need more time to dig into that.

What does the COB_SYSTEM_GEN change do exactly?

It changes the single void * argument to a plain void, so it makes the function not expecting any argument. I haven't checked further why this was needed - just took this from the Gentoo patch.

GitMensch

This may needs a rebase, but it definitely needs a Changelog update :-)
It would also be good to handle review comments.

cobc/codegen.c

GitMensch · 2025-07-28T13:41:54Z

cobc/codegen.c

 	}
 	if (static_call_cache) {
 		const char			*convention_modifier;
+		FILE				*savetarget = output_target;


Please add a comment on line 1898 that we switch that for the output_function_arg_types call.

GitMensch · 2025-07-28T13:42:19Z

cobc/codegen.c

 	sclp->convention = convention;
 	sclp->return_type = return_type;
+	sclp->args = args;
 	sclp->next = static_call_cache;


@ddeclerck Can you check that, please?
If we can pass the prototype instead of its components "return type" + "args", we should do so.

GitMensch · 2025-07-28T13:43:05Z

cobc/codegen.c

+static void
+output_function_arg_types (cb_tree args)
+{
+	cb_tree		x;


new function, so please add a doxygen-style comment here as well

GitMensch · 2025-07-28T21:06:41Z

All parts but cobc were adjusted upstream and that seems to build fine with gcc-15 now - but fails a bunch of internal checks, which will likely be solved by the changes here; please rebase.

Note: I'm not sure that casting the funcint in cob_call_field to all possible argument numbers is really a good idea... I guess it may "crash harder" in the case that the function does not take any arguments than specifying none explicit... on some architectures.

GitMensch · 2025-07-29T11:12:27Z

@ddeclerck Thanks for finishing that!

ddeclerck · 2025-07-29T13:54:28Z

Of course, now that I rebased, test 27 fails. Seems there's a clash between the generated declaration and the one provided through the >>IMP directive...

Note sure what to do here: is it the responsability of the user to provide prototypes compatible with the generated ones ? Should we give him an option to disable the generated prototypes (so he'd have to provide a prototype for each function) ? Something else ?

GitMensch · 2025-07-29T14:20:01Z

Of course, now that I rebased, test 27 fails. Seems there's a clash between the generated declaration and the one provided through the >>IMP directive...

Note sure what to do here: is it the responsability of the user to provide prototypes compatible with the generated ones ? Should we give him an option to disable the generated prototypes (so he'd have to provide a prototype for each function) ? Something else ?

If the user provides a prototype, then it should be "correct".
If the user make use of -fno-gen-c-decl-static-call then we shouldn't generate them - which is still the case, no?

ddeclerck · 2025-07-29T15:16:41Z

If the user provides a prototype, then it should be "correct". If the user make use of -fno-gen-c-decl-static-call then we shouldn't generate them - which is still the case, no?

I had completely forgotten about the -fno-gen-c-decl-static-call option. Adding this to the offending test works fine.

Still have to tackle the other comments.

ddeclerck force-pushed the std23_fixes branch from 4b0782f to 8b23dfe Compare May 13, 2025 09:05

ddeclerck requested a review from GitMensch May 13, 2025 09:09

ddeclerck mentioned this pull request May 13, 2025

Allow configuring the diff command #227

Closed

ddeclerck force-pushed the std23_fixes branch from 8b23dfe to d23694c Compare May 13, 2025 14:37

GitMensch requested changes May 13, 2025

View reviewed changes

GitMensch requested changes Jul 28, 2025

View reviewed changes

GitMensch assigned ddeclerck Jul 29, 2025

ddeclerck force-pushed the std23_fixes branch 2 times, most recently from 5fa94b8 to 3b7bba2 Compare July 29, 2025 13:22

Fixes for C23

d774c1e

ddeclerck force-pushed the std23_fixes branch from 3b7bba2 to d774c1e Compare July 29, 2025 14:48

		static void
		output_function_cast (struct cb_call p, size_t ret_ptr, const char convention, const char *name_str)

Fixes for C23 #229

Are you sure you want to change the base?

Fixes for C23 #229

Uh oh!

Conversation

ddeclerck commented May 12, 2025

Uh oh!

ddeclerck commented May 13, 2025

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitMensch Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitMensch May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeclerck commented May 13, 2025

Uh oh!

GitMensch commented May 13, 2025

Uh oh!

ddeclerck commented May 13, 2025

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitMensch commented Jul 28, 2025

Uh oh!

GitMensch commented Jul 29, 2025

Uh oh!

ddeclerck commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GitMensch commented Jul 29, 2025

Uh oh!

ddeclerck commented Jul 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

GitMensch Jul 29, 2025 •

edited

Loading

GitMensch May 13, 2025 •

edited

Loading

ddeclerck commented Jul 29, 2025 •

edited

Loading