[v4,4/4] build: enable MSVC specific compiler options

Message ID 1691778287-15746-5-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series enable use of the MSVC compiler |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation fail meson build failure
ci/Intel-compilation fail Compilation issues
ci/github-robot: build fail github build: failed

Commit Message

Tyler Retzlaff Aug. 11, 2023, 6:24 p.m. UTC
  * Enable optional use of C11 atomics support.
* Enable use of C23 typeof operator.
* Explicitly force intrinsics when building with MSVC.
* Disable MSVC C runtime checks.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 config/meson.build | 8 ++++++++
 1 file changed, 8 insertions(+)
  

Comments

Bruce Richardson Aug. 14, 2023, 8:30 a.m. UTC | #1
On Fri, Aug 11, 2023 at 11:24:47AM -0700, Tyler Retzlaff wrote:
> * Enable optional use of C11 atomics support.
> * Enable use of C23 typeof operator.
> * Explicitly force intrinsics when building with MSVC.
> * Disable MSVC C runtime checks.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

If there is going to be a lot of this type of special handling for MSVC, we
could look to add a separate config/msvc (and config/gcc-like) directory
with separate meson.build files for the different toolchains. Might help
centralize all such definitions in one place rather than having
conditionals everywhere.

/Bruce

> ---
>  config/meson.build | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/config/meson.build b/config/meson.build
> index 821a1c3..839057a 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -27,6 +27,14 @@ dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
>  is_ms_compiler = is_windows and (cc.get_id() == 'msvc')
>  is_ms_linker = is_windows and (cc.get_id() == 'clang' or is_ms_compiler)
>  
> +# MS compiler (except x86) does not support inline assembly
> +if is_ms_compiler
> +    dpdk_conf.set('_CRT_SECURE_NO_WARNINGS', 1)
> +    dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
> +    add_project_arguments('/experimental:c11atomics', language: 'c')
> +    add_project_arguments('/d1experimental:typeof', language: 'c')
> +endif
> +
>  # set the major version, which might be used by drivers and libraries
>  # depending on the configuration options
>  pver = meson.project_version().split('.')
> -- 
> 1.8.3.1
>
  
Tyler Retzlaff Aug. 14, 2023, 4:10 p.m. UTC | #2
On Mon, Aug 14, 2023 at 09:30:20AM +0100, Bruce Richardson wrote:
> On Fri, Aug 11, 2023 at 11:24:47AM -0700, Tyler Retzlaff wrote:
> > * Enable optional use of C11 atomics support.
> > * Enable use of C23 typeof operator.
> > * Explicitly force intrinsics when building with MSVC.
> > * Disable MSVC C runtime checks.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> If there is going to be a lot of this type of special handling for MSVC, we
> could look to add a separate config/msvc (and config/gcc-like) directory
> with separate meson.build files for the different toolchains. Might help
> centralize all such definitions in one place rather than having
> conditionals everywhere.

i think that would probably be a good idea. it would untangle the
toolchain detail from the flow of the build files.

i don't propose introducing it in this series but when this is merged
i would like to reach out and get your thoughts on how to properly set
up a config/toolchain-xxx. in addition to the compiler flags and
definitions below it would be good to suppress (for now) warnings until
i have an opportunity to evaluate and address the code raising them.
  
Bruce Richardson Aug. 14, 2023, 4:46 p.m. UTC | #3
On Mon, Aug 14, 2023 at 09:10:53AM -0700, Tyler Retzlaff wrote:
> On Mon, Aug 14, 2023 at 09:30:20AM +0100, Bruce Richardson wrote:
> > On Fri, Aug 11, 2023 at 11:24:47AM -0700, Tyler Retzlaff wrote:
> > > * Enable optional use of C11 atomics support.  * Enable use of C23
> > > typeof operator.  * Explicitly force intrinsics when building with
> > > MSVC.  * Disable MSVC C runtime checks.
> > > 
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > If there is going to be a lot of this type of special handling for
> > MSVC, we could look to add a separate config/msvc (and config/gcc-like)
> > directory with separate meson.build files for the different toolchains.
> > Might help centralize all such definitions in one place rather than
> > having conditionals everywhere.
> 
> i think that would probably be a good idea. it would untangle the
> toolchain detail from the flow of the build files.
> 
> i don't propose introducing it in this series but when this is merged i
> would like to reach out and get your thoughts on how to properly set up a
> config/toolchain-xxx. in addition to the compiler flags and definitions
> below it would be good to suppress (for now) warnings until i have an
> opportunity to evaluate and address the code raising them.
> 
Agree on not requiring it for this set. I'm not exactly sure how to split
up the toolchain files, especially given that gcc and clang (and other
llvm-based compilers like icx) are so very, very similar in what we have to
do for them. It would be very wasteful to have individual toolchain files
for each one, duplicating lots of settings. That's why my initial
suggestion was for msvc and "gcc-like" compilers. Any suggestions for a
better name for the latter, welcome! :-)
  
Morten Brørup Aug. 14, 2023, 6:28 p.m. UTC | #4
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 14 August 2023 18.47
> 
> On Mon, Aug 14, 2023 at 09:10:53AM -0700, Tyler Retzlaff wrote:
> > On Mon, Aug 14, 2023 at 09:30:20AM +0100, Bruce Richardson wrote:
> > > On Fri, Aug 11, 2023 at 11:24:47AM -0700, Tyler Retzlaff wrote:
> > > > * Enable optional use of C11 atomics support.  * Enable use of C23
> > > > typeof operator.  * Explicitly force intrinsics when building with
> > > > MSVC.  * Disable MSVC C runtime checks.
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > >
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > If there is going to be a lot of this type of special handling for
> > > MSVC, we could look to add a separate config/msvc (and config/gcc-
> like)
> > > directory with separate meson.build files for the different
> toolchains.
> > > Might help centralize all such definitions in one place rather than
> > > having conditionals everywhere.

Please think twice before splitting into per-toolchain files! You must consider the ratio of differences vs. similarities.

<rant on>
For reference, consider the EAL, which has per-arch subdirectories basically containing the same files, whereof many contain nearly similar content for the different architectures.
And much of it is not even arch-specific at all, but plain C.
There is way too much copy-paste in this design pattern.
Although a "default fallback" implementation could alleviate some of this, minor arch-specific deviations in a function would still require a full copy-paste of the function's remaining code.
In my opinion, a lot of this would be better having in unified files with #ifdefs where required by one or more architectures. It would also allow arch A and arch B to share one implementation, while arch C and arch D share another, and arch E has its own.
<rant off>

The EAL might be a really bad comparison here, but it's a good example of a differences/similarities ratio moving in an unfavorable direction for the design pattern.
I also hope it serves as an example of how not to do it.

That being said, I admit that it could probably be done right. In this case, it's a mostly matter of taste if you prefer keeping the differences together in one file, or having the differences spread out at the locations where they have their actual effect.

> >
> > i think that would probably be a good idea. it would untangle the
> > toolchain detail from the flow of the build files.
> >
> > i don't propose introducing it in this series but when this is merged
> i
> > would like to reach out and get your thoughts on how to properly set
> up a
> > config/toolchain-xxx. in addition to the compiler flags and
> definitions
> > below it would be good to suppress (for now) warnings until i have an
> > opportunity to evaluate and address the code raising them.
> >
> Agree on not requiring it for this set. I'm not exactly sure how to
> split
> up the toolchain files, especially given that gcc and clang (and other
> llvm-based compilers like icx) are so very, very similar in what we have
> to
> do for them. It would be very wasteful to have individual toolchain
> files
> for each one, duplicating lots of settings. That's why my initial
> suggestion was for msvc and "gcc-like" compilers. Any suggestions for a
> better name for the latter, welcome! :-)
  
David Marchand Aug. 15, 2023, 1:21 p.m. UTC | #5
Hello Tyler,

On Fri, Aug 11, 2023 at 8:24 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> * Enable optional use of C11 atomics support.
> * Enable use of C23 typeof operator.
> * Explicitly force intrinsics when building with MSVC.
> * Disable MSVC C runtime checks.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  config/meson.build | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/config/meson.build b/config/meson.build
> index 821a1c3..839057a 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -27,6 +27,14 @@ dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
>  is_ms_compiler = is_windows and (cc.get_id() == 'msvc')
>  is_ms_linker = is_windows and (cc.get_id() == 'clang' or is_ms_compiler)
>
> +# MS compiler (except x86) does not support inline assembly

One last (hopefully) small question, what does this comment apply to?
Forced use of intrinsics?

> +if is_ms_compiler
> +    dpdk_conf.set('_CRT_SECURE_NO_WARNINGS', 1)
> +    dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
> +    add_project_arguments('/experimental:c11atomics', language: 'c')
> +    add_project_arguments('/d1experimental:typeof', language: 'c')
> +endif
> +
>  # set the major version, which might be used by drivers and libraries
>  # depending on the configuration options
>  pver = meson.project_version().split('.')
> --
> 1.8.3.1
>
  

Patch

diff --git a/config/meson.build b/config/meson.build
index 821a1c3..839057a 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -27,6 +27,14 @@  dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
 is_ms_compiler = is_windows and (cc.get_id() == 'msvc')
 is_ms_linker = is_windows and (cc.get_id() == 'clang' or is_ms_compiler)
 
+# MS compiler (except x86) does not support inline assembly
+if is_ms_compiler
+    dpdk_conf.set('_CRT_SECURE_NO_WARNINGS', 1)
+    dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
+    add_project_arguments('/experimental:c11atomics', language: 'c')
+    add_project_arguments('/d1experimental:typeof', language: 'c')
+endif
+
 # set the major version, which might be used by drivers and libraries
 # depending on the configuration options
 pver = meson.project_version().split('.')