Minor patches for the C code in Merlin #1998
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Oh wow! I think it deserve a Changelog entry ! |
Collaborator
Contributor
Author
Ah yes, I think this patch will fix the warning encountered by the user; but the build failure is unrelated. This patch series doesn't apply cleanly to v4.7-413, unfortunately. The opam files for these old merlin version are missing constraints on menhir. # syntax=docker/dockerfile:1
FROM ocaml/opam:ubuntu-25.04-ocaml-4.13
RUN sudo ln -sf /usr/bin/opam-2.4 /usr/bin/opam && opam init --reinit -ni
ADD --keep-git-dir --link --chown=1000:1000 https://github.com/ocaml/merlin.git#v4.7-413 /home/opam/merlin
RUN git config --global --add safe.directory ~/merlin
WORKDIR /home/opam/merlin
RUN --mount=type=cache,target=/home/opam/.opam/download-cache,sharing=locked,uid=1000,gid=1000 \
opam install -y --deps-only --with-dev --formula='"menhir" {<= "20220210"}' merlin.4.7-413
RUN patch -p1 <<'EOF'
diff --git a/src/frontend/ocamlmerlin/dune b/src/frontend/ocamlmerlin/dune
index faa01b93..cc549032 100644
--- a/src/frontend/ocamlmerlin/dune
+++ b/src/frontend/ocamlmerlin/dune
@@ -14,7 +14,7 @@
(modules (:standard \ gen_ccflags))
(libraries config yojson merlin_analysis merlin_kernel
merlin_utils os_ipc ocaml_parsing query_protocol query_commands
- ocaml_typing ocaml_utils seq))
+ ocaml_typing ocaml_utils))
(executable
(name gen_ccflags)
EOF
ADD --chown=1000 --link https://github.com/ocaml/merlin/pull/1998.diff 1998.diff
RUN patch -p1 < 1998.diff
RUN opam exec -- make -j1 |
Contributor
Author
Could you do that for me? 🙏 |
MisterDA
force-pushed
the
ocamlmerlin.c
branch
from
a45515a to
600f43e
Compare
Contributor
Author
|
Marking as draft because the CI hasn't run, and there are errors with my patches on Windows. I'll circle back to this. |
On Windows, use [GetTempPath2][] if available or [GetTempPath][] to retrieve the temporary directory path. On other systems, follow the [XDG Base Directory Specification][XDG] and put the socket file in the XDG runtime directory. It is indicated by the `XDG_RUNTIME_DIR` environment variable, that is always set on platforms implementing the spec. Otherwise, fallback to `TMPDIR` or `/tmp`. [GetTempPath2]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppath2a [GetTempPath2]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha [XDG]: https://specifications.freedesktop.org/basedir/latest/
MisterDA
force-pushed
the
ocamlmerlin.c
branch
5 times, most recently
from
18a16e8 to
94dba5b
Compare
MisterDA
changed the title
ocamlmerlin.c
Contributor
Author
|
Ready again! |
voodoos
reviewed
Nov 26, 2025
In C, arrays passed as arguments to functions decay into pointers, and
the size information is lost. This means all of these are equivalent:
void f(char *s);
void f(char s[]);
void f(char s[3615]);
One way to partially retain this information is to use `static n` with
`n` being the array size: the compiler now knows that the first `n`
elements after the pointed address can be accessed. This C99 feature
is unfortunately not supported by MSVC (but supported by clang-cl).
The compiler uses this info to check that the caller supplies a long
enough buffer to the callee. The size information is not used inside
the callee.
- make the function static; - don't use K&R-style declaration, compilers now reject it before C23. After C23 `f(void)` is similar to `f()`.
Quoting POSIX 2024: > The size of sun_path is required to be constant, but intentionally > does not have a specified name for that constant. Historically, > different implementations used different sizes. For example, 4.3 BSD > used a size of 108, and 4.4 BSD used a size of 104. Since most > implementations originate from BSD versions, the size is typically > in the range 92 to 108. An application can deduce the size by using > `sizeof(((struct sockaddr_un *)0)->sun_path)`.
See also [`sys/un.h`][1] and [unix(7)][2]. [1]: https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/sys_un.h.html [2]: https://man7.org/linux/man-pages/man7/unix.7.html
It works with mingw-w64 too.
warning: '__p__environ' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Variable-Attributes.html#index-dllimport-variable-attribute
to count the number of elements in an array.
`perror` can be used with functions from the CRT that set `errno`, otherwise `GetLastError` and `FormatMessage` have to be used instead.
_snprintf doesn't null-terminate strings. MSVCRT doens't provide snprintf, but if __USE_MINGW_ANSI_STDIO is defined, then mingw-w64 will provide this function. There are environments where mingw-w64 is configured with MSVCRT, and others where it is configured with UCRT. It seems safer to use the compatibility shims. UCRT provides snprintf, and MSVC defaults to UCRT.
MisterDA
force-pushed
the
ocamlmerlin.c
branch
from
94dba5b to
4a3dd2f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I did a review of
ocamlmerlin.c. I found two issues:struct sockaddr_un. On some BSD systems thestruct sockaddrfamily of types have a leadingsxx_lenfield. It was not taken into account. Useoffsetof, as described by POSIX, to compute the socklen.https://man.freebsd.org/cgi/man.cgi?unix(4)
https://man7.org/linux/man-pages/man7/unix.7.html
https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/sys_un.h.html
errno, and their failure code can be retrieved withGetLastErrorand the error string withFormatMessage. Functions from the CRT do seterrno, andperrorcan be used to display the error string.There's also a few patches that improve the code quality, in my opinion. It now assumes a C99 compiler with (optionally) C2y extensions. For reference, the OCaml runtime requires at least C11.