Add a COB_LOAD_GLOBAL config option to modify `dlopen` behavior #209

florianschmidt1994 · 2025-01-08T15:00:33Z

This PR adds a config option called COB_LOAD_GLOBAL which allows the flags of dlopen used in call.c to be changed between RTLD_GLOBAL and RTLD_LOCAL (see https://linux.die.net/man/3/dlopen)

Motivation

This flag is motivated by an ambition to be able to write a multi-threaded c program, that then loads libcob with dlmopen in one namespace per thread, effectively allowing us to call the same libcob / Cobol module from multiple threads in a "thread-safe" way. The image below shows the a very simplified intended call sequence

Because there is a "bug" in dlopen that stops us from calling dlopen("module", RTLD_GLOBAL), this PR introduces an option to be able to change this flag to RTLD_LOCAL at runtime.

As at glibc 2.24, specifying the RTLD_GLOBAL flag when calling dlmopen() generates an error. Furthermore, specifying RTLD_GLOBAL when calling dlopen() results in a program crash (SIGSEGV) if the call is made from any object loaded in a namespace other than the initial namespace.

Docs

# Environment name:  COB_LOAD_GLOBAL
#   Parameter name:  load_global
#          Purpose:  tell the system loader to provide symbols in CALLed
#                    programs globally, allowing symbols to be found for linked
#                    libraries later
#             Type:  boolean
#             Note:  COBOL CALLs will always find symbols in already CALLed or
#                    pre-modules; this setting is mostly an advise to the system,
#                    not all systems are capable of loading libraries global/local
#          Default:  false
#          Example:  load_global true

Implementation

Change lt_dlopen from a macro definition to an actual function, which internally calls dlopen with the flags based on the configuration
Add cob_load_global / COB_LOAD_GLOBAL as a configuration
Add documentation to runtime.cfg about COB_LOAD_GLOBAL

GitMensch · 2025-01-08T15:28:24Z

Apart from the other PR's review to that branch, the change here makes the current scenario even more confusing.
I'd like to fix this in this PR (or up-front upstream).

After the changes:

instead of calling lt_dlopen we call a new static function cob_dlopen
this has the current code of this PR in for the case of dlopen usage
for the WIN32 variant it uses what is currently used there
for the libtool variant it uses lt_dlopenadvise() with an advise matching the runtime setting

@florianschmidt1994 can you please inspect the libtool variant (to test this you can simply undefine USE_LIBDL in config.h)?
I think I'll work on the cob_dlopen stuff late-evening.

florianschmidt1994 · 2025-01-08T16:09:48Z

Yup, I'll check it out. Basically something like this, right?

static void* cob_dlopen(const char* filename) {
	#ifdef _WIN32
	if (x == NULL) {
		return GetModuleHandle (NULL);
	}
	return LoadLibrary(x);	
	#elif	defined(USE_LIBDL)
	int flags = cobsetptr->cob_load_global 
		? RTLD_LAZY | RTLD_GLOBAL
		: RTLD_LAZY | RTLD_LOCAL;

	return dlopen(filename, flags);
	# else
	// TODO: initialise lt_dladvise
	lt_dlopenadvise(filename, /*advise here*/);
}

florianschmidt1994 · 2025-01-08T16:39:09Z

@GitMensch I've tried out your suggestion in the latest commit and temporarily defined lt_dlopen as cob_dlopen to try it out. I've also added some indentation to the macros, does that conform with your style or should I remove them again?

GitMensch

nice update, we may be able to push that soon

Note: we still need the testcase addition - does not need to be something that benefit from this as this is hard to make portable - just do a simple CALL and CANCEL. We should have an easy one for COB_LOAD_CASE

ChangeLog

libcob/call.c

libcob/coblocal.h

libcob/call.c

config/runtime.cfg

libcob/call.c

tests/testsuite.src/run_misc.at

florianschmidt1994 · 2025-01-13T09:41:22Z

When developing, I've noticed some test failures, where some of them might be related to our change. On the latest commit of this repositories "main branch" (gcos4gnucobol-3.x), there are some test failures when the build is configured with --without-dl. These failures persist also with our changes applied.

Commit Hash	Loader	COB_LOAD_GLOBAL	Test Failures
`731b81a`	--without-dl	not applicable	47 49 50 784 failed
`731b81a`		not applicable	no test failure

In the first version of this PR (bd80e8c), there is an additional test failure (781) when using COB_LOAD_GLOBAL=false, i.e. the default, and preloading where symbols are overwritten in an unexpected manner.

Commit Hash	Loader	COB_LOAD_GLOBAL	Test Failures
`bd80e8c`		false	781 failed
`bd80e8c`		true	no test failure
`bd80e8c`	--without-dl	false	47 49 50 781 failed
`bd80e8c`	--without-dl	true	47 49 50 failed

In history up to the latest commit (62de625), there exists a fix (see 1f4d607), the same that is already applied in WIN32 environments, to fix this.

Commit Hash	Loader	COB_LOAD_GLOBAL	Test Failures
`62de625`		false	no test failure
`62de625`		true	no test failure
`62de625`	--without-dl	false	47 49 50 failed
`62de625`	--without-dl	true	47 49 50 failed

using the more important part "default changed" first, more explanation

codecov-commenter · 2025-02-11T19:01:32Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 67.58%. Comparing base (16e84a5) to head (3234374).
Report is 44 commits behind head on gcos4gnucobol-3.x.

Files with missing lines	Patch %	Lines
libcob/call.c	70.58%	2 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #209      +/-   ##
=====================================================
- Coverage              67.85%   67.58%   -0.28%     
=====================================================
  Files                     33       33              
  Lines                  60458    60827     +369     
  Branches               15821    15896      +75     
=====================================================
+ Hits                   41026    41109      +83     
- Misses                 13567    13847     +280     
- Partials                5865     5871       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

GitMensch

I hope to reinspect that PR soon, ideally after looking over CANCEL in general.

Can you please do the minor and cosmetic changes outlined in this review and rebase afterwards?

GitMensch · 2025-02-11T13:49:12Z

NEWS

@@ -87,6 +87,12 @@ NEWS - user visible changes				-*- outline -*-
   $b for executable basename, $d for date in YYYYMMDD format, $t for time
   in HHMMSS format (before, only $$ was available for pid)

+** introduction of the COB_LOAD_GLOBAL boolean flag, which determines whether


that's a text block with sentences, which needs to start upper-case; but maybe go with a short entry as text, then a double-colon and newline, then more explanatory text (this could stay as sentences) afterwards, in any case the full-stops should be outside of the quotes :-)

GitMensch · 2025-02-11T13:50:07Z

config/ChangeLog

+2024-12-16 Simon Sobisch <[email protected]>
+
+	* config/runtime.cfg: Add documentation for COB_LOAD_GLOBAL flag
+
 2024-08-17 Ammar Almoris <[email protected]>


oops: after the date (also applies to the next entry) there should be two spaces, not one; and "add" should be lower-cased and may be shorted as "add COB_LOAD_GLOBAL"

GitMensch · 2025-02-11T18:33:59Z

libcob/call.c

+	}
+	return LoadLibrary(filename);
+#elif	defined(USE_LIBDL)
+	int flags = cobsetptr->cob_load_global 


should be const int

GitMensch · 2025-02-11T18:34:45Z

libcob/call.c

+    	} else {
+        	error = lt_dladvise_local(&advise);
+    	}


use tab instead of space to indent, please leave a space before opening parenthesis

GitMensch · 2025-02-11T18:36:07Z

libcob/call.c

+    void* handle = lt_dlopenadvise(filename, advise);
+    return handle;


declarations are only allowed at the start of the block with C89, but that isn't really needed here either - just do the same as above, directly return the function call's result

GitMensch · 2025-05-13T18:51:02Z

tests/testsuite.src/run_misc.at

+AT_CHECK([COB_LOAD_GLOBAL=true ./caller], [0], [], [])
+AT_CHECK([COB_LOAD_GLOBAL=false ./caller], [0], [], [])


Both lines should also use $COBCRUN_DIRECT (the test above that you've copied from misses this also, so please adjust there as well).

GitMensch · 2025-05-13T19:01:51Z

tests/testsuite.src/run_misc.at

+AT_CHECK([$COMPILE caller.cob], [0], [], [])
+AT_CHECK([COB_LOAD_GLOBAL=true ./caller], [0], [], [])
+AT_CHECK([COB_LOAD_GLOBAL=false ./caller], [0], [], [])
+


Can you please add both COB_LOAD_GLOBAL options (and its name to the keyword) for the pre-load test to have that covered as well?

GitMensch · 2025-05-13T19:11:08Z

config/runtime.cfg

+#                    libraries later
+#             Type:  boolean
+#             Note:  COBOL CALLs will always find symbols in already CALLed or
+#                    pre-modules; this setting is mostly an advise to the system,


should be "pre-loaded modules"

GitMensch · 2025-05-13T19:18:08Z

libcob/call.c

@@ -289,6 +283,37 @@ static int last_entry_is_working_directory (const char *buff, const char *pstr)
 	return 0;
 }

+static void* cob_dlopen(const char* filename) {
+#ifdef _WIN32


because of the #elif below, please use #if defined (_WIN32) here

GitMensch · 2025-05-13T19:23:41Z

libcob/call.c

+
+	return dlopen(filename, flags);
+#else
+    if (advise != NULL) {


advise is global, so only set if the "last set" variant is different to the current one - you can store the last used cob_load_global as a static var within this function for that

florianschmidt1994 added 3 commits January 8, 2025 13:16

First draft

b4ba590

Add documentation

41e6d95

Change default COB_LOAD_GLOBAL to false

eab2b74

GitMensch mentioned this pull request Jan 8, 2025

WIP: Feature/add dlopen config flag florianschmidt1994/gnucobol#1

Closed

florianschmidt1994 added 2 commits January 8, 2025 15:19

Move option in config_tbl to similar options

142f7f9

Remove wrong NULL check

e783e37

florianschmidt1994 added 2 commits January 8, 2025 15:46

Address review comments

d81423a

Add entry to ChangeLog

e7705d2

refactor and introduce a cob_dlopen function

505f8c9

GitMensch requested changes Jan 8, 2025

View reviewed changes

florianschmidt1994 added 2 commits January 9, 2025 09:23

Restructure according to PR comments

9dd8c17

:Remove unnecessary whitespace changes

ab26e16

florianschmidt1994 force-pushed the feature/add-dlopen-config-flag branch from f6aa350 to ab26e16 Compare January 9, 2025 09:31

florianschmidt1994 added 6 commits January 9, 2025 09:53

Address PR comment

2e9315b

Move changelog entries to respective folders

6f11f38

Improve handling of lt_dladvise

bd80e8c

Resolve symbol overwrite issue

1f4d607

Add entry to NEWS

8aed811

Add simple testcase

62de625

GitMensch reviewed Jan 9, 2025

View reviewed changes

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

florianschmidt1994 added 2 commits January 13, 2025 10:09

Merge test-cases

d109c59

COB_LOAD_GLOBAL

5e7dc77

florianschmidt1994 changed the title ~~WIP: Add a COB_LOAD_GLOBAL config option to modify dlopen behavior~~ Add a COB_LOAD_GLOBAL config option to modify dlopen behavior Jan 13, 2025

florianschmidt1994 marked this pull request as ready for review January 13, 2025 10:24

florianschmidt1994 added 2 commits February 11, 2025 11:43

Merge branch 'gcos4gnucobol-3.x' into feature/add-dlopen-config-flag

992ad2f

Fix issue with upper / lower case call

a5bf8a6

florianschmidt1994 and others added 3 commits February 11, 2025 13:27

Fix another case issue

709ca21

Remove C++ style comment

14232c6

Update ChangeLog

ea0a8bf

using the more important part "default changed" first, more explanation

Merge branch 'gcos4gnucobol-3.x' into feature/add-dlopen-config-flag

3234374

florianschmidt1994 force-pushed the feature/add-dlopen-config-flag branch from 8871e45 to 3234374 Compare March 18, 2025 09:36

GitMensch requested changes May 13, 2025

View reviewed changes

		void* handle = lt_dlopenadvise(filename, advise);
		return handle;

		AT_CHECK([COB_LOAD_GLOBAL=true ./caller], [0], [], [])
		AT_CHECK([COB_LOAD_GLOBAL=false ./caller], [0], [], [])

Add a COB_LOAD_GLOBAL config option to modify dlopen behavior #209

Are you sure you want to change the base?

Add a COB_LOAD_GLOBAL config option to modify dlopen behavior #209

Uh oh!

Conversation

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

Uh oh!

Motivation

Docs

Implementation

Uh oh!

GitMensch commented Jan 8, 2025

Uh oh!

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

Uh oh!

Uh oh!

florianschmidt1994 commented Jan 8, 2025

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

codecov-commenter commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add a COB_LOAD_GLOBAL config option to modify `dlopen` behavior #209

Add a COB_LOAD_GLOBAL config option to modify `dlopen` behavior #209

florianschmidt1994 commented Jan 8, 2025 •

edited

Loading

florianschmidt1994 commented Jan 8, 2025 •

edited

Loading

florianschmidt1994 commented Jan 13, 2025 •

edited

Loading

codecov-commenter commented Feb 11, 2025 •

edited

Loading