Don't use configured CFLAGS & CPPFLAGS to compile third-party C sources#12589
Don't use configured CFLAGS & CPPFLAGS to compile third-party C sources#12589dra27 merged 5 commits intoocaml:trunkfrom
Conversation
With my Debian hat on: this reliance is accidental. Instead, the rule is to use CFLAGS et al. at package build time. It is populated automatically by the Debian toolchain during package building, but users are free to set it to whatever they like for non-system software. However, it turns out that these variables may be ignored by the build system, see for example ocaml/dune#8464. |
We provide invalid CFLAGS and CPPFLAGS for the OCaml compiler to make sure these flags are not used while building the compiler itself.
We provide invalid CFLAGS and CPPFLAGS for the OCaml compiler to make sure these flags are not used while building the compiler itself.
We provide invalid CFLAGS and CPPFLAGS for the OCaml compiler to make sure these flags are not used while building the compiler itself.
|
Adding @jamesjer too for Fedora This change does seem totally backwards. We want the hardening flags to be passed through when compiling 3rd party software (or a way for us to be able to specify them to be passed through). It has been an ongoing problem that there is no way to pass specific flags for the compiler, and for what the compiler passes through, which we have attempted to solve with downstream patching in the past eg this commit (which we have since dropped): |
|
Also this commit (also now dropped): https://pagure.io/fedora-ocaml/c/05b882737e0daecb86a6ef54e192508c60efcd88?branch=fedora-37-4.14.0 |
|
@rwmjones so AIUI there are two needs: (1) the need for a way _not_ to
have those CFLAGS used to compile OCaml propagate to the compilation of
third-party code and (2) (the need you expressed) to be able to specify,
at OCaml configuration time, C compiler flags that _will_ be used when
ocamlc or ocamlopt are used to compile third-party C sources. Is that a
correct sumary?
If so: I do think both needs can be fulfilled. The current PR does not
fulfill them in its present form, and I think your needs can be
addressed in a separate PR because they are distinct.
In terms of how to address them, the best way I can think would be to
add an `OCAMLC_CFLAGS` configuration variable where you could put the
flags you wnat to hardcode into the compiler. Would that seem a right
approach?
|
|
Yes, that seems better. Jerry what do you think? As background to this we need to run |
Maybe this does not need to be done in one go, but I would prefer to see a "better approach to pass C compilation flags to the OCaml compilers" merged before (or at the same time that) we remove the current mechanism that (some) distributions rely on. |
|
Gabriel Scherer (2023/09/21 03:58 -0700):
> The current PR does not fulfill them in its present form, and I think your needs can be addressed in a separate PR because they are distinct.
Maybe this does not need to be done in one go, but I would prefer to
see a "better approach to pass C compilation flags to the OCaml
compilers" merged before (or at the same time that) we remove the
current mechanism that (some) distributions rely on.
I can try. But first I need to make sure we do pass CFLAGS everywhere in
our own build system. On trunk we were propagating them too far, so to
speak, but now with the current implementaiotn we do'nt propagate them
far enough. That's the first thing to stamibise, I think. And I
uderstandthat, if this gets merged before the next release, then we will
also need to merge something that makes it possible to pass flags to be
used by the compiler. Of course user programs always have the
possibility to pass options manually with `-ccopt` but I do understand
it's highly non-trivial to convince a user package's build system of
doing that.
|
|
I agree with @rwmjones 's comments. I want to point out that we want to set both compiler and linker flags, including invocations of the linker from ocamlmklib. |
It seems also more coherent to have the flag just before the name of the file that is being compiled.
The CFLAGS should not be included there, and -D_XPG6 should be in preprocessor rather than compiler flags.
3b99c57 to
aff1d6c
Compare
|
I just pushed an updated version of this PR. The purpose of the PR remains the same, i.e. making sure that the C compiler/preprocessor flags passed to @rwmjones: this PR takes into account your comment by introducing new configuration variables: This new version of the PR thus superseeds #12975 by taking into account #13200 and #13201. The revised implementation starts with a commit ensuring that the The core of the work lives in the PR's second commit. A third commit fixes the configuration for the Sun C compiler where the CFLAGS were dealt with whereas they should not have been. Finally, this updated version of the PR fixes a duplication of flags related to pthread in the build system that has been reported by @tmcgilchrist (thanks!). It was causing no harm but was not very elegant either. @jamesjer at this stage I am unsure how to address your comment. Is the current support of |
Changes
Outdated
|
|
||
| * #12589: Use configured CFLAGS and CPPFLAGS *only* during the build of | ||
| the compiler itself. Do not use them when compiling third-party | ||
| C sources trhough the compiler. Flags for compiling third-party C |
There was a problem hiding this comment.
| C sources trhough the compiler. Flags for compiling third-party C | |
| C sources through the compiler. Flags for compiling third-party C |
aff1d6c to
238c66c
Compare
|
Fixed! Thanks!
|
|
The duplication of |
|
The flags you added for @rwmjones sound like what I am interested in. Thank you for that. Are the flags passed to the linker by ocamlmklib affected, or just flags passed by ocamlc and ocamlopt? I would like to have some way to set those passed by ocamlmklib, if possible. |
|
Tim McGilchrist (2024/06/24 15:33 -0700):
The duplication of `pthread` is fixed, thank you @shindere.
Thanks a lot for having double checked!
One small nitpick, a few variables have extra whitespace padding that
could be trimmed. [...]
Indeed. It turns out that avoiding superfluous spaces in flag lists is non-trivial as you must check, each time you add a flag to a list, whether the original list is empty (in whihc case the sesulting list is just that flag), or not (in which case the resulting list is the original one plus a space plus the flag).
Although we do make efforts to avoid superfluous spaces, this is not done systematically yet and we often add a space before the flag, without checking whether this is necessary or not, hence all these spaces you see.
One possible improvement consists in using the [AX_APPEND_FLAG](http://www.gnu.org/software/autoconf-archive/ax_append_flag.html) and [AX_PREPEND_FLAG](http://www.gnu.org/software/autoconf-archive/ax_prepend_flag.html) macros provided as part of the [GNU Autoconf archive](http://www.gnu.org/s/autoconf-archive/). These macros do not only avoid the superfluous spaces, they also check whether an added flag is already present in the variable to which it gets added to avoid repeating it.
I will be happy to submit a PR implementing this improvement, but would rather keep it distinct from the current one as the aims are different, and wait until this one has been merged to avoid the rebase hurdle, as the two PRs have code portions in common.
In any case, many thanks for the note @tmcgilchrist, which motivated me to look into this in more details.
For the sake of completeness, let me also say that, even if the varialbes themselves stop containing superfluous spaces, it may still be the case that the commands output by `make` will contain some, e.g. when certain variables are empty. It will thus be helpful at some point to make sure that all the commands output by make are first passed to its [strip](https://www.gnu.org/s/make/manual/html_node/Text-Functions.html) function so that they get normalized, which will make it easier to compare build logs by avoiding false positive due to changes in spacing.
PS: it's too bad GH does not interprete Markdown when replying by e-mail.
|
|
Whitespace is cosmetic and shouldn't make any difference, I wouldn't worry about it. |
|
Jerry James (2024/06/24 19:56 -0700):
The flags you added for @rwmjones sound like what I am interested in.
Thank you for that.
Good. Thanks for confirming.
Are the flags passed to the linker by ocamlmklib affected, or just
flags passed by ocamlc and ocamlopt?
I am not sure I understand your question, since ocamlmklib is just a
wrapper. In the end, it does call ocamlc and ocamlopt.
I would like to have some way to set those passed by ocamlmklib, if
possible.
Are you able to be more precise here and give a concrete example of what
you need and that does not work yet?
FWIW, ocamlmklib can't be given C files as input. Also, it is just a
wrapper around ocamlopt and ocamlc so it ends up calling them.
For what I can tell, the flags put into the `LDFLAGS` configure variable
will be embedded in the compiler and used both internally (when we use
the compiler to compile our own tools) and externally, when the compiler
is used to compile (here link!) user programs.
|
|
Thanks for the comprehensive work and the bugfix! AFAICT this successfully prevents CFLAGS options from being passed to the user. With this pull request on the latest trunk, as desired. |
|
thanks a lot for having taken the time to checkthat the fix was actually
working and to reporting very clearly about your experiment.
|
|
I've put the PR through precheck just to test the couple of more obscure platforms available there, but good to merge once Changes is updated and that has passed. |
238c66c to
e443814
Compare
This is a leftover from 31cdf41, (part of ocaml#12589).
This is a leftover from 31cdf41, (part of ocaml#12589).
This is a leftover from 31cdf41, (part of ocaml#12589).
This is a leftover from 31cdf41, (part of ocaml#12589).
This is a leftover from 31cdf41, (part of ocaml#12589).
…iables They had been renamed in 31cdf41 (part of ocaml#12589) but we should keep the old names for backward compatibility.
…iables They had been renamed in 31cdf41 (part of ocaml#12589) but we should keep the old names for backward compatibility.
…iables They had been renamed in 31cdf41 (part of ocaml#12589) but we should keep the old names for backward compatibility.
…iables They had been renamed in 31cdf41 (part of ocaml#12589) but we should keep the old names for backward compatibility.
…ables They had been renamed in 31cdf41 (part of ocaml#12589) but we should keep the old names for backward compatibility.
When C flags or C preprocessor flags are passed to configure, they
should be used to build the compiler itself only, not when the
compiler is invoked to compile third-party C sources, like stubs.
The way flags were computed for the Sun C compiler looked suspiscious,
so it gets updated here, too.