fletcher86 commented 5 years ago

Expected Behavior

If user supplies '/Z{i|I|7}', it should take precedence over over hard coded options and not give a warning. Likewise if user supplies '/O{1|2|b|d|g|i|s|t|x|y}', it should take precedence over hard coded options.

Current Behavior

For visual studio native builds, when user specifies compile options '/Z7' & '/Od', hard coded values from within the source code, '/Zi' and '/Od' get passed to the compiler causing warning when compiling code.

Context

The hard coded values interfere with user supplied arguments creating warning in the output logs. See file:///E:/compiler-options/build/tmp/compileDebugCpp/output.txt for all output for compileDebugCpp. compiling main.cpp successful. main.cpp cl : Command line warning D9025 : overriding '/Zi' with '/Z7'

Steps to Reproduce (for bugs)

git clone https://github.com/fletcher86/compiler-options.git cd compiler-options gradle build

Then examine output.

cl : Command line warning D9025 : overriding '/Zi' with '/Z7'

Your Environment

Build scan URL: https://scans.gradle.com/s/eus3ekqemxft4

big-guy commented 5 years ago

Let's provide an answer this week.

fletcher-sumglobal commented 5 years ago

We can generate a pull request for this because we have a code fix for it. We came up with the following solution after some lengthy discussions and would like to hear your feedback.

Our sponsor wanted to express the the debug and optimization levels as separate DSL expressions along side compilerArg DSL. Our solution involves two new properties that are provider api enabled and are read in by the AbstractNativeCompileTask. The properties override hard coded values '/Zi' and '/O2' if present. If not present, existing defaults are set. The two new properties are as follows:

debugInfoFormat optimizationLevel

PR to follow.

lacasseio commented 5 years ago

Some discussions are available here: https://github.com/gradle/gradle/pull/7409#discussion_r226014087

lacasseio commented 5 years ago

Optimization

There is two controls over the optimization level: high level and low level.

High Level

The high level is more about what does the user requires? Microsoft breaks it down as: disabled, minimize size or maximize speed. While GCC breaks it down as: none, speedy (some optimization), no space-speed tradeoff, all optimization, minimize size, maximize speed and debugging experience. Gradle will most likely not model after a specific toolchain but instead, it should model around the use case. For this, here are a couple of open questions to answer:

What does a debugging experience optimization level look like? No optimization? Is this what the user is searching when configuring his debugging session?
Does the average user care about "some" optimization and no space-speed tradeoff optimization or does he care more about minimizing size, maximizing speed?

My feeling is the optimization level should have 4 level: disabled/none, no space-speed tradeoff, minimize size and maximize speed. I think those 4 level cover most use cases. The default for debugging should be disabled/none, and for release, it should be no space-speed tradeoff.

Low Level

The low level is more about the tweaking the various optimization algorithm/feature. At this level, the users know (or should know) exactly what he is doing. I would classify this as out of scope for now until we have valid use case of a real project where this is important.

Debuggable

MSVC has three different debug options: embedded, PDB and PDB with edit and continue. GCC and Clang are more complicated. It has several format options: dwarf, stabs, stabs with GNU extension, xcoff, xcoff with GNU extension and gvms. It also has a meta-option that choose the most expressive for GDB. All debugging information with GCC and Clang are embedded. You can strip and extract the debugging information in an external file using objcopy and strip.

$ objcopy --only-keep-debug a.out a.dbg
$ strip ./a.out

My feeling is we should have 3 level of debuggability: disabled/none, embedded, side-by-side. In the case of MSVC, embedded would be /Z7 and side-by-side would be either ZI for unoptimized binaries and Zi for anything else because Microsoft mentioned the following in the guide "Because most optimizations are incompatible with Edit and Continue, using /ZI disables any #pragma optimize statements in your code." As for GCC/Clang, we should use -g for both embedded or side-by-side. Specifically for side-by-side, Gradle should execute the right command to strip the symbols out into a separated file.

amz-shahji commented 5 years ago

@lacasseio That's nicely summarized.

Your suggestion for optimization flags cover probably /O1, /O2, and /Od (and may be a few others). What about all the other optimization options on this page here
How do these play with compilers outside of MSVS, GCC or Clang. We are building for gaming consoles that might not use either of these compilers. There are teams still using Watcom and CodeWarrior :)

lacasseio commented 5 years ago

Awww good old CodeWarrior. The high-level model should be kind of how MSVC and GCC maps /O1, etc to specific lower-level optimization flags. For Watcom and CodeWarrior, those high-level model would translate to something meaningful for these compilers. CodeWarrior may be harder as the concepts are more rudimentary. For those cases, where there is no equivalent, we can adopt a fail-fast behavior meaning providing a meaningful error regarding a configuration Gradle just can't figure out for the developer according to what they are asking. We could also downgrade to another option such as no optimize for speed but no trade-off optimization exists, we could use that one instead with a warning. These should ultimately be configurable.

As for the low-level configuration flags, either those are manually passed through the command line. In that case, we could have a 5th model option which would be "override". The reason for this 5th option is to warn and potentially disallow manual optimization flags for all other levels. The ultimate model would be to expose a custom model per toolchain type and version with only the options available that a user can toggle on or off. The nice thing with such modeling is having in IDE documentation and URL back to toolchain documentation of each option through the Kotlin DSL.

Another shorter term solution we discussed for your need may be to provide a way to set on the CppCompile task the effective compile arguments to use for debuggable and optimized booleans. It's quite similar to PR https://github.com/gradle/gradle/pull/7437 to the exception of not exposing the option through the DSL. It would respect our separation of what (DSL)/how (task) and we can deprecate the option once the model is more flesh out.

fletcher-sumglobal commented 5 years ago

@lacasseio Not sure I follow everything in your last post. I should say I understand the long term vision for strong modeling per toolchain. However, for the short term, If I understand you correctly, I think you are giving us the nod to move forward with #7437 with some exceptions? Can you explain a little more how those exceptions would work in comparison to the current implementation?

fletcher-sumglobal commented 5 years ago

@lacasseio in reference to the discussion on https://github.com/gradle/gradle/pull/7409, and to a certain extent in your previous comment, you stated that

Given that, maybe a better solution without modeling anything would be to switch the CppCompile task boolean for debuggable and optimized false, then you can pass whatever argument through the compileArgs.

and @amz-shahji responded.

I might be missing something with your suggestion here. Those are boolean flags and defaulted to false already. The issue is when set false they resolve to /Wi and /O0 correspondingly still. They resolve differently when true.

What I think you are saying is modify the code such that when those two flags are set to false, then allow user supplied compilerArgs to override hard coded values? When set to true, then allow gradle to dictate debug/optimization flags? This will affect the unit tests obviously. I just wanted clarification on exact behavior before moving forward, because this would mean rolling back much of what is currently in the PR https://github.com/gradle/gradle/pull/7437. Specifically removal of support for the following DSL below, removal of changes to the spec, etc.

debugInfoFormat = '/Z7'
optimizationLevel = '/O3'

Please confirm your intentions on expected behavior.

zosrothko commented 5 years ago

From my perpective, Gradle should not hard code any of the tuning options like '/Zi' & '/O2' for VisualCpp and I am in favor of removing the hard coded options for all compilers. Gradle DSL offers all capabilities with Flavor to setup those options.

zosrothko commented 5 years ago

BTW, why Gradle does not simply remove the commented lines below from the VisualCppCompilerArgsTransformer<T extends NativeCompileSpec>class.

Since MicrosoftVisualCppCompilerPluginhas the @Incubating annotation, there is no firm contract to be upward compatible? Or may be I a missing something?

    protected void addToolSpecificArgs(T spec, List<String> args) {
        args.add(getLanguageOption());
        args.add("/nologo");
        args.add("/c");
/*
        if (spec.isDebuggable()) {
            args.add("/Zi");
        }
        if (spec.isOptimized()) {
            args.add("/O2");
        }
*/
    }

fletcher-sumglobal commented 5 years ago

@zosrothko removal of the code you have commented would certainly solve my immediate needs to provide a short term solution. However, I'm not certain what depends on the presence of this code other than the unit tests or existing projects if any.

lacasseio commented 5 years ago

@zosrothko We do want to move forward with stronger modeling as there are several advantages to so. However, while we roll out a stronger model, there will be a time like these where it will cause some friction and we are aware of them. We do want to make a clearer distinction of between the what (DSL) and how (task). Removing the pointed code would be steps backward in what we are trying to achieve.

I’m a bit hesitant to suggest moving toward https://github.com/gradle/gradle/pull/7437 without the DSL modification because of the added property specifically for the optimization and debug format level. Those are most likely not how we want to model things in the future and may go away. A counter suggestion would be to define 3 flags for debuggable/optimizable where the 3rd one is OVERRIDE. It would remove all hardcoded values and expect the compilerArgs to contains the right flags. The binary may or may not be optimized. It would be left to the user to decide what the development binary and production binary means in term of compiler flags and disambiguation rule during resolution. It may require to formally define those two binaries as such instead of debug and release.

@build-tool-native-team Thoughts?

amz-shahji commented 5 years ago

@lacasseio @zosrothko An override option is just as good. We are fine with anything that will allow us to decide the parameters at the DSL level. Of course, we don't want to write/maintain million lines of code to achieve this, but anything reasonable is just as good. For our use case we want to define everything explicitly in DSL, most specifically to ensure visibility into what the parameters are. As long as we can achieve that, the means is less important to us. For us this is going to be a global setting across all projects so that we explicitly set every parameter and is visible to the user in the script. No hidden truths!!

Given the history/past experience with build systems, this is a strong acceptance requirement for the transition.

fletcher-sumglobal commented 5 years ago

Well if there are no objections, i'll introduce a 3rd boolean OVERRIDE allowing user to explicit set compiler args. This should simplify the PR greatly.

fletcher-sumglobal commented 5 years ago

Updated PR 7437 by introducing 3rd boolean overrideCompilerArgs allowing user to override hard coded compiler args -> https://github.com/gradle/gradle/pull/7437/commits/14e0d3ee3d7cca20ec412b495a0cffa8fdbed3af . The DSL now looks like the following with optional DSL overrideCompilerArgs:

tasks.withType(CppCompile) {

    overrideCompilerArgs = true
    compilerArgs.addAll toolChain.map { NativeToolChain toolChain ->
        List<String> compilerSpecificArgs = []
        if (toolChain instanceof VisualCpp) {
            compilerSpecificArgs << '/TP'
            compilerSpecificArgs << '/nologo'
            compilerSpecificArgs << '/c'
            compilerSpecificArgs << '/Z7'
            compilerSpecificArgs << '/Od'
        }
        return compilerSpecificArgs
    }
}

Also removed previous proposed implementation.

lacasseio commented 5 years ago

Thanks for the update on this, I commented on the PR with the POC resulting from the internal discussion we had. I'm still missing some feedback from the @gradle/native team to confirm the direction of the implementation.

The general consensus is to avoid any flags within the model DSL. Model element would be passed to the tool chain for transformation. Ultimately, you will be able to create your own optimization model to represent your specific needs.

To share configuration throughout your projects/organization, you would create a convention plugin that would expose all your specific organizational optimization model and register the hooks on the tool chain to transform those models into the compiler flags to use. You could also piggyback on the default optimization model provided by Gradle and just override the compiler flags with a tool chain hook.

fletcher-sumglobal commented 5 years ago

I've been spending quite a bit of time trying to get my head wrapped around the POC and my initial observation is that it only addresses one compiler option, namely the optimization flag, '/O?' with no consideration for the debug flag, '/Z?'. However, I understand this is still a work in progress. The build complexity seems to be growing exponentially for a single compiler flag. Not sure how this solution will look when you include the '/Z?' debug option. Secondly, your integration test uses the deprecated software model which we have been told to avoid.

Then you recommend creating a convention plugin:

you would create a convention plugin that would expose all your specific organizational optimization model and register the hooks on the tool chain to transform those models into the compiler flags to use. You could also piggyback on the default optimization model provided by Gradle and just override the compiler flags with a tool chain hook

I'm not sure how this is accomplished. Is there an example somewhere on how to create a 'convention' plugin from a native transform context? The documentation on implementing plugins says two things that caught my attention:

Please avoid using the Convention concept when writing new plugins. The long term plan is to migrate all Gradle core plugins to use extensions and remove the Convention concept altogether

Please be aware that the "convention mapping" API is undocumented and might be removed with later versions of Gradle

Without any documentation or anything concrete to follow, this is all esoteric to me at this point. I will continue to study it to see if I can get a better understanding.

gradle / gradle-native

Hard coded visual studio compiler options '/Zi' (debug information format) & '/O2' (optimization level). Need capability to override these hard coded values. #884

Expected Behavior

Current Behavior

Context

Steps to Reproduce (for bugs)

Your Environment

Optimization

High Level

Low Level

Debuggable