cmake: Simplify compiler options with Cable #770

chfast · 2021-03-24T13:18:10Z

No description provided.

codecov · 2021-03-24T13:55:14Z

Codecov Report

Merging #770 (3f4f4ad) into master (eeca732) will increase coverage by 0.13%.
The diff coverage is n/a.

❗ Current head 3f4f4ad differs from pull request most recent head 1260d7b. Consider uploading reports for the commit 1260d7b to get more accurate results

@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
+ Coverage   99.14%   99.28%   +0.13%     
==========================================
  Files          80       77       -3     
  Lines       12374    11953     -421     
==========================================
- Hits        12268    11867     -401     
+ Misses        106       86      -20

Flag	Coverage Δ
rust	`99.89% <ø> (-0.01%)`	⬇️
spectests	`90.55% <ø> (+<0.01%)`	⬆️
unittests	`99.22% <ø> (+0.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/fizzy/execute.cpp	`98.71% <0.00%> (-0.83%)`	⬇️
test/unittests/capi_test.cpp	`99.87% <0.00%> (-0.03%)`	⬇️
bindings/rust/src/lib.rs	`99.89% <0.00%> (-0.01%)`	⬇️
lib/fizzy/execute.hpp	`100.00% <0.00%> (ø)`
lib/fizzy/instantiate.cpp	`100.00% <0.00%> (ø)`
lib/fizzy/instantiate.hpp	`100.00% <0.00%> (ø)`
test/unittests/api_test.cpp	`100.00% <0.00%> (ø)`
test/unittests/wasi_test.cpp	`100.00% <0.00%> (ø)`
test/utils/execute_helpers.hpp	`100.00% <0.00%> (ø)`
test/unittests/execute_test.cpp	`100.00% <0.00%> (ø)`
... and 11 more

axic · 2021-03-29T16:59:32Z

CMakeLists.txt

    -Wcast-qual
    -Wcast-align
    -Wmissing-declarations
-    $<$<COMPILE_LANGUAGE:CXX>:-Wextra-semi>
-    $<$<COMPILE_LANGUAGE:CXX>:-Wold-style-cast>
+    IF_SUPPORTED


So anything below the IF_SUPPORTED magic is only enabled if the compiler supports it?

How about ONLY_IF_SUPPORTED?

Yes, these are optional and enabled if compiler of a particular language supports it.

axic · 2021-03-29T17:00:11Z

CMakeLists.txt

    -Wcast-qual
    -Wcast-align
    -Wmissing-declarations
-    $<$<COMPILE_LANGUAGE:CXX>:-Wextra-semi>
-    $<$<COMPILE_LANGUAGE:CXX>:-Wold-style-cast>


Does COMPILE_LANGUAGE:CXX means if the file is cpp or if the compiler is run in c++ mode?

It means C++ compilation determined by CMake by the source file extension (the fact that this is the same tool for C and C++ is only coincidence).

axic · 2021-03-29T17:01:05Z

cmake/CableCompileOptions.cmake

+            set(result_var "${lang}${flag_id}")
+
+            # Check if the flag works in the lang's compiler.
+            # In CMake 3.18+ cmake_language(CALL ...) can be used.


This means the file does not require 3.18+? What version is required, can we put a min version on the top?

I don't know what the minimal CMake version required is. Hard to tell without testing it with multiple versions. And I don't think it is possible to put cmake_minimum_required() in this file because this will affect the project that loads this file.

axic · 2021-03-29T18:17:13Z

CMakeLists.txt

-cable_add_cxx_compiler_flag_if_supported(-Wduplicated-cond)
-cable_add_cxx_compiler_flag_if_supported(-Wduplicate-enum)
-cable_add_cxx_compiler_flag_if_supported(-Wlogical-op)
-cable_add_cxx_compiler_flag_if_supported(-Wno-unknown-attributes)

 if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
    option(WEVERYTHING "Enable almost all compiler warnings" OFF)


Would it make sense having another helper/case, i.e.

cable_add_compile_option( -Weverything IF_PRESENT # If the first option(s) works try adding all these? -Wno-c++98-compat ...

Not very easy to guess what it does IMHO

What exactly?

@axic's suggestion. Your change looks fine I think.

axic · 2021-05-19T10:16:11Z

Since the holdup here is to check if this works with Solidity, there we'd need something like this:

cable_add_compile_options(
        IF_SUPPORTED
        -Wimplicit-fallthrough
        # Prevent the path of the source directory from ending up in the binary via __FILE__ macros.
        -fmacro-prefix-map=${CMAKE_SOURCE_DIR}=/solidity
        # -Wpessimizing-move warns when a call to std::move would prevent copy elision
        # if the argument was not wrapped in a call.  This happens when moving a local
        # variable in a return statement when the variable is the same type as the
        # return type or using a move to create a new object from a temporary object.
        -Wpessimizing-move
        # -Wredundant-move warns when an implicit move would already be made, so the
        # std::move call is not needed, such as when moving a local variable in a return
        # that is different from the return type.
        -Wredundant-move
)

Would this work?

axic · 2021-05-19T10:26:20Z

And how would you simplify the following using the new option:

        eth_add_cxx_compiler_flag_if_supported(-fstack-protector-strong have_stack_protector_strong_support)
        if(NOT have_stack_protector_strong_support)
                eth_add_cxx_compiler_flag_if_supported(-fstack-protector)
        endif()

chfast · 2021-05-19T10:50:51Z

Would this work?

This work.

And how would you simplify the following using the new option:

        eth_add_cxx_compiler_flag_if_supported(-fstack-protector-strong have_stack_protector_strong_support)
        if(NOT have_stack_protector_strong_support)
                eth_add_cxx_compiler_flag_if_supported(-fstack-protector)
        endif()

This cannot be replaced.

chfast · 2021-05-19T11:06:19Z

This is not documented, but -fstack-protector overrides the -fstack-protector-strong. Still you can safely do -fstack-protector -fstack-protector-strong.
https://godbolt.org/z/xPsW179eK

So simplified version would be

cable_add_compile_options(IF_SUPPORTED -fstack-protector -fstack-protector-strong)

chfast · 2021-05-19T11:10:34Z

Other good news is that -fstack-protector-strong is supported in all GCC/Clang versions that support -std=c++17 so you can ignore -fstack-protector if you want -fstack-protector-strong.
https://godbolt.org/z/zq1P78jMh

axic · 2021-05-19T11:29:47Z

Here are the potential changes/simplifications in Solidity: https://github.com/ethereum/solidity/tree/cmake-options

chfast requested a review from axic March 24, 2021 13:18

chfast mentioned this pull request Mar 24, 2021

cable_add_cxx_compiler_flag_if_supported breaks options that rely on slashes being preserved ethereum/cable#15

Open

chfast force-pushed the cable_add_compile_options branch from e4a1d0c to fc4a3e4 Compare March 24, 2021 21:48

axic reviewed Mar 29, 2021

View reviewed changes

axic approved these changes Mar 29, 2021

View reviewed changes

chfast force-pushed the cable_add_compile_options branch from 3f4f4ad to c177dd3 Compare May 19, 2021 12:54

chfast marked this pull request as ready for review May 19, 2021 12:56

chfast added 2 commits May 20, 2021 13:21

cable: Add CableCompilerOptions

47044d8

cmake: Simplify compiler options with Cable

1260d7b

chfast force-pushed the cable_add_compile_options branch from c177dd3 to 1260d7b Compare May 20, 2021 11:22

axic approved these changes May 20, 2021

View reviewed changes

chfast merged commit 2fe6f02 into master May 20, 2021

chfast deleted the cable_add_compile_options branch May 20, 2021 12:45

axic mentioned this pull request May 20, 2021

Simplify add_compile_options in CMake ethereum/solidity#11422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Simplify compiler options with Cable #770

cmake: Simplify compiler options with Cable #770

chfast commented Mar 24, 2021

codecov bot commented Mar 24, 2021 •

edited

Loading

axic Mar 29, 2021

chfast Mar 29, 2021

axic Mar 29, 2021

chfast Mar 29, 2021

axic Mar 29, 2021

chfast Mar 29, 2021

axic Mar 29, 2021

gumb0 Apr 8, 2021

chfast Apr 8, 2021

gumb0 Apr 8, 2021

axic commented May 19, 2021

axic commented May 19, 2021

chfast commented May 19, 2021

chfast commented May 19, 2021

chfast commented May 19, 2021 •

edited

Loading

axic commented May 19, 2021

cmake: Simplify compiler options with Cable #770

cmake: Simplify compiler options with Cable #770

Conversation

chfast commented Mar 24, 2021

codecov bot commented Mar 24, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented May 19, 2021

axic commented May 19, 2021

chfast commented May 19, 2021

chfast commented May 19, 2021

chfast commented May 19, 2021 • edited Loading

axic commented May 19, 2021

codecov bot commented Mar 24, 2021 •

edited

Loading

chfast commented May 19, 2021 •

edited

Loading