-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use strict prototypes on primitives in bytecomp #12509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7385175
to
e718a27
Compare
It does, so I've removed this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted, thanks! You may have constified less than you thought, see below.
The bytecode compiler also extracts the list of primitives to a C file, similarly as the Makefile. The Makefile was fixed in 8db1817 to account for stricter C function prototypes, but the bytecomp problems were not discovered back then. As the startup code includes `caml/mlvalues.h`, two primitives are declared twice with conflicting types. We prevent this with preprocessor tricks before including the header. The generated C code now also uses the same names as in the Makefile.
e718a27
to
99a05d2
Compare
Thanks for the review, I've applied your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The #define
dance for the two declared primitives is not great but does the job, and I cannot think of a simpler solution. Good to merge!
@Octachron It's a bit late, but: can we consider this for 5.1 too? Note that the same patch for the compiler, in the compiler, has been merged since January in #11861. Apple clang 14 defaults to GNU C 17, and generates warnings for this error every time a bytecode executable is built. This can be quite confusing to users, and often pollutes the result of test suites using bytecode executable on macOS.
|
Considering that this is the kind of changes that would warrant an opam patch on the compiler for the sake of compatibility, I agree that this should be cherry-picked to 5.1.0 . |
Use strict prototypes on primitives when building standalone bytecode executable (cherry picked from commit 2ecd298)
Use strict prototypes on primitives when building standalone bytecode executable (cherry picked from commit 2ecd298)
Use strict prototypes on primitives when building standalone bytecode executable (cherry picked from commit 2ecd298)
Use strict prototypes on primitives when building standalone bytecode executable (cherry picked from commit 2ecd298)
Use strict prototypes on primitives when building standalone bytecode executable (cherry picked from commit 2ecd298)
Use strict prototypes on primitives when building standalone bytecode executable (cherry picked from commit 2ecd298)
Earlier this year, we enabled warnings for strict prototypes in C code (mostly replacing
f()
declarations withf(void)
). This led to fixing the primitives' list generation; see #11763 and #11861 for history.clang has started raising warnings by default when it encounters such K&R declarations.
We didn't catch that there was another place generating the list of primitives: the bytecode compiler when it builds a complete executable!
The first commit brings the bytecomp to generate the code we've agreed on from the previous PRs, with the additional twist that we must prevent two functions from
mlvalues.h
from being re-declared with conflicting types. This is done with some preprocessor tricks inspired by what we already do withATOM
andATOM_WS
.We can't extract the original types of the primitives, the bytecomp uses its
Dll
module to check whether a symbol exists in a dll, and types aren't retained.Side dishes are switching to quoted strings for C code: note how it's nice now. Backslashes can be a bit confusing when dealing with macros, too. edit: although looking at a recent issue, I wonder if this can introduce end-of-line problems between Windows and other systems.
I think we can also constify the primitive lists, right?
I wonder if we can make
caml_data
,caml_code
, andcaml_sections
const
in the generated C code for the complete bytecode executable.