Skip to content
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

Document preferred way to add opt only compile flags with rules based toolchain #325

Open
keith opened this issue Jan 28, 2025 · 6 comments
Assignees

Comments

@keith
Copy link
Member

keith commented Jan 28, 2025

A common pattern in the previous CC toolchain was to have some compile flags split on dbg/fastbuild/opt. I don't see any docs or examples of how to do that with the rule based toolchain. My first attempt was this:

cc_feature_constraint(
    name = "opt_cfg",
    all_of = ["@rules_cc//cc/toolchains/features:opt"],
)

cc_args(
    name = "opt_compile_flags",
    actions = ["@rules_cc//cc/toolchains/actions:compile_actions"],
    args = [
        "-DNDEBUG",
        "-D_FORTIFY_SOURCE=1",
        "-O2",
    ],
    requires_any_of = [":opt_cfg"],
)

But the flags aren't added, so I assume that's not the right move. I found another example where the user just overrode the feature instead:

https://github.com/lowRISC/opentitan/blob/7704c7a75d9fff2fdfb42e40d5bd311ac9f94f9b/toolchain/BUILD#L193-L202

@AustinSchuh
Copy link

AustinSchuh commented Jan 28, 2025

You are prodding in eerily similar code right now to me.

I found it super handy to put a print inside src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl and print out toolchain_config_info. That shows the exact crosstool protocol buffer being fed into bazel, if I'm reading it right. In theory, we should be able to get that to match with both APIs.

The code you have above looks right. That generates:

  flag_set {
    action: "assemble"
    action: "c++-compile"
    action: "c++-header-parsing"
    action: "c++-module-codegen"
    action: "c++-module-compile"
    action: "c++-module-deps-scanning"
    action: "c++20-module-codegen"
    action: "c++20-module-compile"
    action: "c-compile"
    action: "clif-match"
    action: "linkstamp-compile"
    action: "lto-backend"
    action: "preprocess-assemble"                                                                                                       
    flag_group {                                                                                                                                      
      flag: "-DNDEBUG"                                                                                  
      flag: "-D_FORTIFY_SOURCE=1"                                                                                                                                   
      flag: "-O2"                                                                                                                                                                                                               
    }                                                                                                                                           
    with_feature {                                                                                                      
      feature: "opt"                                                                                                               
    }     
  } 

For unix_cc_toolchain_config, the old toolchain definition when adjusted to have the same flags, generates an identical crosstool protocol buffer.

If you change it to requires_any_of = ["@rules_cc//cc/toolchains/features:opt"],, that generates an equivalent crosstool protocol buffer.

I'm trying to convert unix_cc_toolchain_config over to the new rules based toolchain. When comparing the original crosstool with the new one, I see the following only in the working crosstool:

feature {                      
  name: "opt"                                                                       
  enabled: false                                               
} 

This makes me think there is something wrong with the definitions for the external features for "opt" and the others that is somehow preventing them from being created automatically.

@keith
Copy link
Member Author

keith commented Jan 29, 2025

thanks for the debugging tip! that's very useful. with working on the unix toolchain conversion are you removing the reliance on the legacy features? im considering looking into that but if you're already nearly done with that I wouldn't have to!

@armandomontanez armandomontanez self-assigned this Jan 29, 2025
@AustinSchuh
Copy link

Keith, I'm starting with "Port, line for line, the old syntax over to rules". I don't have any legacy features converted right now. Now I know to check in with you before starting so we don't duplicate each other :)

@trybka
Copy link
Collaborator

trybka commented Jan 29, 2025

I found it super handy to put a print inside src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl and print out toolchain_config_info. That shows the exact crosstool protocol buffer being fed into bazel, if I'm reading it right. In theory, we should be able to get that to match with both APIs.

Similar to this but without needing to modify builtins, etc. you can accomplish this with cquery.

e.g.

bazel cquery --output=starlark --starlark:expr 'providers(target).get("CcToolchainConfigInfo").proto' $CC_TOOLCHAIN

If your goals involve a straight port, in theory you could resurrect the comparator tests: https://github.com/bazelbuild/rules_cc/blob/0.0.1/tools/migration/ctoolchain_comparator.py

@keith
Copy link
Member Author

keith commented Jan 29, 2025

that's a great tip! note that for use with the rule based toolchain if you have a toolchain like:

cc_toolchain(
    name = "clang_toolchain",
...

The target you need to use is :_clang_toolchain_config

@keith
Copy link
Member Author

keith commented Jan 29, 2025

It seems like in the proto representation from this i get 3 duplicate entries 1 after the other, any idea how to eliminate that? it just adds some noise to diffs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants