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

Clarify the use of C compiler related variables in the build system. #911

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
5 participants
@shindere
Contributor

shindere commented Nov 13, 2016

Cc @gasche @whitequark @alainfrisch @adrien-n @dra27

This PR is inspired by PR #892 and more specifically by David's
comment at #892 (comment)

So pr #892 DOES ACTUALLY DEPEND ON THIS ONE.

The part of the commit that modifies the makefiles has been reviewed
by @damiendoligez.

Comments will be warmly appreciated, especially regarding the documentation.

@adrien-n

This comment has been minimized.

Show comment
Hide comment
@adrien-n

adrien-n Nov 13, 2016

Contributor

I don't think this should be merged as-is since it would change many things for users of the compiler.

The fact is that today, all libraries are built with $bytecccompopts and dropping these options can impact all of them In particular, options such as -fno-strict-aliasing and -fwrapv can have an impact on the code that is included in ocaml headers with e.g. macros (I haven't checked but it /is/ a possibility). Moreover, flags such as -O2 and -Wall are useful to provide to the build of libraries.

I concur it is better to have compiler-specific options and the current state has bitten me too in the past but we also probably want to keep default ones for the libraries too. Even for C stubs we probably don't want to authors to have to think too much about C and its constraints. Lastly, there can be ABI issues and for instance config/Makefile.mingw{,w64} use {BYTE,NATIVE}CCCOMPOPTS to pass -mms-bitfields which has an impact on ABI; no OCaml library author should have to think about this or everything will break.

(NB: -mms-bitfields has been the default for some time for mingw ports but it doesn't harm and it's more work to make it conditional on the compiler versions that really need it)

I would probably introduce a new COMPILERCOMPOPTS or something like that in order to hold the compiler-specific flags such as -DCAML_NAME_SPACE.

Contributor

adrien-n commented Nov 13, 2016

I don't think this should be merged as-is since it would change many things for users of the compiler.

The fact is that today, all libraries are built with $bytecccompopts and dropping these options can impact all of them In particular, options such as -fno-strict-aliasing and -fwrapv can have an impact on the code that is included in ocaml headers with e.g. macros (I haven't checked but it /is/ a possibility). Moreover, flags such as -O2 and -Wall are useful to provide to the build of libraries.

I concur it is better to have compiler-specific options and the current state has bitten me too in the past but we also probably want to keep default ones for the libraries too. Even for C stubs we probably don't want to authors to have to think too much about C and its constraints. Lastly, there can be ABI issues and for instance config/Makefile.mingw{,w64} use {BYTE,NATIVE}CCCOMPOPTS to pass -mms-bitfields which has an impact on ABI; no OCaml library author should have to think about this or everything will break.

(NB: -mms-bitfields has been the default for some time for mingw ports but it doesn't harm and it's more work to make it conditional on the compiler versions that really need it)

I would probably introduce a new COMPILERCOMPOPTS or something like that in order to hold the compiler-specific flags such as -DCAML_NAME_SPACE.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 13, 2016

Contributor
Contributor

shindere commented Nov 13, 2016

@adrien-n

This comment has been minimized.

Show comment
Hide comment
@adrien-n

adrien-n Nov 13, 2016

Contributor

Adding the options in another variable or in BYTECC and NATIVECC should lead to the same result. As such I have no opinion on which one should be done over the other.

As for -O2 and -Wall, my current concern is that the various build systems for ocaml and the compiler itself haven't really shined when it comes to passing options to the C compiler. I would prefer that this is improved before the set of flags changes.

Contributor

adrien-n commented Nov 13, 2016

Adding the options in another variable or in BYTECC and NATIVECC should lead to the same result. As such I have no opinion on which one should be done over the other.

As for -O2 and -Wall, my current concern is that the various build systems for ocaml and the compiler itself haven't really shined when it comes to passing options to the C compiler. I would prefer that this is improved before the set of flags changes.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 15, 2016

Contributor

The PR has been rebased and updated to take your comments into account,
@adrien-n.

Most options are passed to C compilers.
The only one that are not passed any longer are the warning-related ones.
This has been decided in agreement with @damiendoligez.

@alainfrisch: the SHAREDCCCOMPOPTS variable contained the -O option on the
MinGW port and the -Ox option on the msvc port. This PR removes these
options from the variable. Could you please confirm this is okay?

Contributor

shindere commented Nov 15, 2016

The PR has been rebased and updated to take your comments into account,
@adrien-n.

Most options are passed to C compilers.
The only one that are not passed any longer are the warning-related ones.
This has been decided in agreement with @damiendoligez.

@alainfrisch: the SHAREDCCCOMPOPTS variable contained the -O option on the
MinGW port and the -Ox option on the msvc port. This PR removes these
options from the variable. Could you please confirm this is okay?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 15, 2016

Contributor

SHAREDCCCOMPOPTS variable contained the -O option on the MinGW port and the -Ox option on the msvc port

I guess this is ok, but this will change (silently) how C user code is compiled, no?

Contributor

alainfrisch commented Nov 15, 2016

SHAREDCCCOMPOPTS variable contained the -O option on the MinGW port and the -Ox option on the msvc port

I guess this is ok, but this will change (silently) how C user code is compiled, no?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 15, 2016

Contributor

Alain Frisch (2016/11/15 05:44 -0800):

SHAREDCCCOMPOPTS variable contained the -O option on the MinGW port and the -Ox option on the msvc port

I guess this is ok, but this will change (silently) how C user code is
compiled, no?

On the MinGW port the -O option is already present in the BYTECC
variable, thus there should be no change at all, except that it will be
passed only once instead of twice.

For the msvc port, there is already a -O2 present in BYTECC, so without
this patch cl is called with -O2 -Ox, I don't know which one takes
precedence in this case. With the patch applied -O2 remains. If -Ox is
prefered I can update the PR accordingly, i.e. replace -O2 by -Ox in
BYTECC.

Contributor

shindere commented Nov 15, 2016

Alain Frisch (2016/11/15 05:44 -0800):

SHAREDCCCOMPOPTS variable contained the -O option on the MinGW port and the -Ox option on the msvc port

I guess this is ok, but this will change (silently) how C user code is
compiled, no?

On the MinGW port the -O option is already present in the BYTECC
variable, thus there should be no change at all, except that it will be
passed only once instead of twice.

For the msvc port, there is already a -O2 present in BYTECC, so without
this patch cl is called with -O2 -Ox, I don't know which one takes
precedence in this case. With the patch applied -O2 remains. If -Ox is
prefered I can update the PR accordingly, i.e. replace -O2 by -Ox in
BYTECC.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 15, 2016

Contributor

If -Ox is prefered

@dra27 Do you have an opinion?

Contributor

alainfrisch commented Nov 15, 2016

If -Ox is prefered

@dra27 Do you have an opinion?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 15, 2016

Contributor

It should possibly be explicitly highlighted in Changes but, realistically, this would only be being used for C stubs in user code, surely? So I can't think optimisation levels are likely to be that critical (both in terms of code generated and code emitted) - so why not go with -Ox?

Contributor

dra27 commented Nov 15, 2016

It should possibly be explicitly highlighted in Changes but, realistically, this would only be being used for C stubs in user code, surely? So I can't think optimisation levels are likely to be that critical (both in terms of code generated and code emitted) - so why not go with -Ox?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 15, 2016

Contributor
Contributor

shindere commented Nov 15, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 15, 2016

Member

For reference, using -O2 instead of just -O for gcc/clang was discussed and decided in #226. There was no mention of MSVC in this discussion. O2 in MSVC actually appeared earlier than that, in @damiendoligez's commit 20c278b which references MPR#6590, in which Damien carefully considered the issue and selected -O2 -Gy- (is that a thing?) going forward.

Member

gasche commented Nov 15, 2016

For reference, using -O2 instead of just -O for gcc/clang was discussed and decided in #226. There was no mention of MSVC in this discussion. O2 in MSVC actually appeared earlier than that, in @damiendoligez's commit 20c278b which references MPR#6590, in which Damien carefully considered the issue and selected -O2 -Gy- (is that a thing?) going forward.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 15, 2016

Contributor
Contributor

shindere commented Nov 15, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 15, 2016

Member

I have no competence to review the PR in general, but on the specific choice of O2/Ox, MPR#6590 seems to suggest that -Ox is now deprecated, so -O2 is probably the better default.

Member

gasche commented Nov 15, 2016

I have no competence to review the PR in general, but on the specific choice of O2/Ox, MPR#6590 seems to suggest that -Ox is now deprecated, so -O2 is probably the better default.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 15, 2016

Contributor

-Ox is not deprecated, but there advice to choose between -O1 or -O2 (see MSDN) which shows I should have checked MSDN before commenting previously (sorry)!

I guess the key thing is whether the testsuite has been fixed subsequently, or whether the -O2 -Ox in BYTECC has masked the issue since 4.02. Given that both -O2 and -Ox are aliases for other flags, I expect this has been fine as presumably it's come out as

  • -O2
  • -Gy- (disabling the -Gy in -O2, I think)
  • -Ox (which adds no new options not already present)

Given that Appveyor's passing (and we've released two versions of OCaml since based on this), it would seem that -O2 -Gy- is working fine?

Bear in mind when looking for comparisons with gcc that -O1 and -O2 are just different strategies - it's not strictly implying that -O2 is more aggressive in the way that gcc's -O option works (where higher n does imply more attempts to optimise).

Contributor

dra27 commented Nov 15, 2016

-Ox is not deprecated, but there advice to choose between -O1 or -O2 (see MSDN) which shows I should have checked MSDN before commenting previously (sorry)!

I guess the key thing is whether the testsuite has been fixed subsequently, or whether the -O2 -Ox in BYTECC has masked the issue since 4.02. Given that both -O2 and -Ox are aliases for other flags, I expect this has been fine as presumably it's come out as

  • -O2
  • -Gy- (disabling the -Gy in -O2, I think)
  • -Ox (which adds no new options not already present)

Given that Appveyor's passing (and we've released two versions of OCaml since based on this), it would seem that -O2 -Gy- is working fine?

Bear in mind when looking for comparisons with gcc that -O1 and -O2 are just different strategies - it's not strictly implying that -O2 is more aggressive in the way that gcc's -O option works (where higher n does imply more attempts to optimise).

@adrien-n

This comment has been minimized.

Show comment
Hide comment
@adrien-n

adrien-n Nov 15, 2016

Contributor

Looks good to me but note I can't say much about the MSVC part. My only question is whether we have (roughly) equivalent warning flags between MSVC and others (others have -Wall -Wno-unused, MSVC has nothing) but that's very minor.

Contributor

adrien-n commented Nov 15, 2016

Looks good to me but note I can't say much about the MSVC part. My only question is whether we have (roughly) equivalent warning flags between MSVC and others (others have -Wall -Wno-unused, MSVC has nothing) but that's very minor.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 15, 2016

Contributor

@adrien-n - indeed, the issue of warnings in MSVC also came up discussions around #912. The equivalent I think is -Wall -wd4100 -wd4101 -wd4102... I'm just running a few tests.

Contributor

dra27 commented Nov 15, 2016

@adrien-n - indeed, the issue of warnings in MSVC also came up discussions around #912. The equivalent I think is -Wall -wd4100 -wd4101 -wd4102... I'm just running a few tests.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 15, 2016

Contributor

Wowzers - I thought that might happen! There's a reason that -Wall is not used in the MSVC ports 😄. There's quite a list of warnings which would need to be gone through and turned off!

Contributor

dra27 commented Nov 15, 2016

Wowzers - I thought that might happen! There's a reason that -Wall is not used in the MSVC ports 😄. There's quite a list of warnings which would need to be gone through and turned off!

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 15, 2016

Contributor

David Allsopp (2016/11/15 10:05 -0800):

Wowzers - I thought that might happen! There's a reason that -Wall is
not used in the MSVC ports 😄. There's quite a list of warnings
which would need to be gone through and turned off!

Thanks a lot for having tested, David!

Going through those warnings, trying to fix them etc. is definitely on
my list for the very short term future, hopefully still this week.

And many thanks for the warnings you have suggested, will start from
there.

Will also use your suggestion for optimization options.

You mentionned testsuite failures. What were you refering to, exactly?

Contributor

shindere commented Nov 15, 2016

David Allsopp (2016/11/15 10:05 -0800):

Wowzers - I thought that might happen! There's a reason that -Wall is
not used in the MSVC ports 😄. There's quite a list of warnings
which would need to be gone through and turned off!

Thanks a lot for having tested, David!

Going through those warnings, trying to fix them etc. is definitely on
my list for the very short term future, hopefully still this week.

And many thanks for the warnings you have suggested, will start from
there.

Will also use your suggestion for optimization options.

You mentionned testsuite failures. What were you refering to, exactly?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 15, 2016

Contributor

The Mantis report mentioned testsuite failures - but I think these were fixed by the addition of -Gy-. Note that Microsoft C includes a lot of warnings which are in fact linting issues, so not necessarily "to be fixed". It looks to me like aspiring to -W3 with a few -Wdn is the best result. Lots of the warnings look very opinionated or deal with corner cases that don't matter (e.g. complaining that unary minus is applied to an unsigned value in the Tag_val macro!)

Contributor

dra27 commented Nov 15, 2016

The Mantis report mentioned testsuite failures - but I think these were fixed by the addition of -Gy-. Note that Microsoft C includes a lot of warnings which are in fact linting issues, so not necessarily "to be fixed". It looks to me like aspiring to -W3 with a few -Wdn is the best result. Lots of the warnings look very opinionated or deal with corner cases that don't matter (e.g. complaining that unary minus is applied to an unsigned value in the Tag_val macro!)

Clarify and slightly modify the use of C compiler related make variab…
…les.

Before this commit, there was no distinction between the options
used to compile C source files coming with the OCaml distribution
and third-party C source files compiled by calling ocamlc or ocamlopt.

This commit makes it possible to use options when compiling C source
files that come with OCaml without imposing these options to the compilation
of third-party code.

More specifically, the options in the BYTECCCOMPOPTS and NATIVECCCOMPOPTS
variables are not passed to the C compiler when called by ocamlc and
ocamlopt any longer.

This commit also documents the role of each concerned variable.

In addition:

- On Unix:
  * The -Wall and -Werror options are no longer passed to the C
    compiler by ocamlc and ocamlopt for third-party C source files

- For the MinGW port:
  * The -O option has been removed from the SHAREDCCCOMPOPTS variable
  * The -Wall and -Wno-unused options are no longer passed to the C
    compiler by ocamlc and ocamlopt for third-party C source files

- For the msvc port: the
  * The -Ox option has been removed from the SHAREDCCCOMPOPTS variable.
  * The -Wall and -Wno-unused options are no longer passed to the C
    compiler by ocamlc and ocamlopt for third-party C source files
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 16, 2016

Contributor
Contributor

shindere commented Nov 16, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 16, 2016

Contributor

adrien-n (2016/11/15 09:10 -0800):

Looks good to me but note I can't say much about the MSVC part. My
only question is whether we have (roughly) equivalent warning flags
between MSVC and others (others have -Wall -Wno-unused, MSVC has
nothing) but that's very minor.

So just to make sure everything is written out clearly: no, at the
moment the msvc build does not have equivalent warnings set up. But this
is supposed to come very soon.

Contributor

shindere commented Nov 16, 2016

adrien-n (2016/11/15 09:10 -0800):

Looks good to me but note I can't say much about the MSVC part. My
only question is whether we have (roughly) equivalent warning flags
between MSVC and others (others have -Wall -Wno-unused, MSVC has
nothing) but that's very minor.

So just to make sure everything is written out clearly: no, at the
moment the msvc build does not have equivalent warnings set up. But this
is supposed to come very soon.

@gasche gasche merged commit 48967c7 into ocaml:trunk Nov 16, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 16, 2016

Member

Given that there seems to be a consensus, I went ahead and merged. Thanks!

Member

gasche commented Nov 16, 2016

Given that there seems to be a consensus, I went ahead and merged. Thanks!

@shindere shindere deleted the shindere:clarify-c-compiler-variables branch Nov 16, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 16, 2016

Contributor
Contributor

shindere commented Nov 16, 2016

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 7, 2016

Contributor

Moving towards using -W3 in the msvc ports is related to MPR#5079

Contributor

dra27 commented Dec 7, 2016

Moving towards using -W3 in the msvc ports is related to MPR#5079

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #911 from shindere/clarify-c-compiler-variables
Clarify the use of C compiler related variables in the build system.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment