Skip to content

avoid overriding the user's OCAMLRUNPARAM settings#83

Merged
tmcgilchrist merged 1 commit into
tarides:mainfrom
gasche:avoid-overriding-OCAMLRUNPARAM
Apr 9, 2026
Merged

avoid overriding the user's OCAMLRUNPARAM settings#83
tmcgilchrist merged 1 commit into
tarides:mainfrom
gasche:avoid-overriding-OCAMLRUNPARAM

Conversation

@gasche

@gasche gasche commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

I wanted to use olly latency to measure the GC latency of a given program under different minor-heap-size settings. This does not work currently because olly overrides the OCAMLRUNPARAM variable. The present PR ensures that the OCAMLRUNPARAM value is extended rather than forgotten.

@gasche

gasche commented Mar 9, 2026

Copy link
Copy Markdown
Contributor Author

Note: I have not actually tested the control-flow path where the user sets OCAMLRUNPARAM and also sets the -log-wsize option. (In part because I'm not sure how to test it, it does not look supported by olly gc-stats.) But the path where olly does not set OCAMLRUNPARAM when log-wsize is not set already fixes the issue for me.

@tmcgilchrist

Copy link
Copy Markdown
Collaborator

I was concerned about how OCaml parses OCAMLRUNPARAM. From looking at startup_aux.c:98-161, the parser iterates left-to-right. Each key (like e) calls scanmult() which unconditionally overwrites the param. Comma is the separator (line 155). So for OCAMLRUNPARAM="e=10,e=20", last value wins (e would be 20). So olly's setting takes precedence over user defined values, as expected.

The other issue is with duplicate env var entries, which isn't introduced here but I've gone back and read the POSIX spec for it. Happily linux glibc and macOS libsystem both scan linearly and return the first matching env var name so we get the olly one.


The relevant section is POSIX.1-2017 Base Definitions, Chapter 8.1 "Environment
Variables"
:

It is defined with type char **environ. ... Environment variable names and values are
strings.
The array of strings is called the environment. The strings have the form name=value ...
the name shall not contain the character '='.
If more than one string in the environment of a process has the same name, the consequences
are undefined.

This means getenv() is free to return any of the duplicate entries. A conforming implementation could return the last, or any arbitrary one. How very useful!


This fix is fine, I'll restructure all the env variable handling to do the right thing.

@tmcgilchrist tmcgilchrist merged commit 48de51c into tarides:main Apr 9, 2026
7 checks passed
@gasche

gasche commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks!

tmcgilchrist added a commit to tmcgilchrist/opam-repository that referenced this pull request Apr 10, 2026
CHANGES:

* Reinstate olly latency command. (tarides/runtime_events_tools#86, @tmcgilchrist)
* Avoid overriding the user's OCAMLRUNPARAM settings. (tarides/runtime_events_tools#83, @gasche)
* Remove help subcommand, as it relies on cmdliner internals. (tarides/runtime_events_tools#81, @theAlexes)
* Fix cmdliner 2.0 compatibility. (tarides/runtime_events_tools#80, @krfantasy)
* Log path when create_cursor raises an exception. (tarides/runtime_events_tools#66, @tmcgilchrist)
* Add option to specify runtime events dir and log size. (tarides/runtime_events_tools#66, @tmcgilchrist)
tmcgilchrist added a commit to tmcgilchrist/opam-repository that referenced this pull request Apr 10, 2026
CHANGES:

* Reinstate olly latency command. (tarides/runtime_events_tools#86, @tmcgilchrist)
* Avoid overriding the user's OCAMLRUNPARAM settings. (tarides/runtime_events_tools#83, @gasche)
* Remove help subcommand, as it relies on cmdliner internals. (tarides/runtime_events_tools#81, @theAlexes)
* Fix cmdliner 2.0 compatibility. (tarides/runtime_events_tools#80, @krfantasy)
* Log path when create_cursor raises an exception. (tarides/runtime_events_tools#66, @tmcgilchrist)
* Add option to specify runtime events dir and log size. (tarides/runtime_events_tools#66, @tmcgilchrist)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants