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

Split C and C++ compilation #1366

Merged
merged 9 commits into from Apr 18, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go/private/actions/archive.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def emit_archive(go, source=None):
runfiles = runfiles.merge(a.runfiles)
if a.source.mode != go.mode: fail("Archive mode does not match {} is {} expected {}".format(a.data.label, mode_string(a.source.mode), mode_string(go.mode)))

if len(extra_objects) == 0 and source.cgo_archive == None:
if len(extra_objects) == 0 and not source.cgo_archives:
go.compile(go,
sources = split.go,
importpath = source.library.importmap,
Expand All @@ -77,7 +77,7 @@ def emit_archive(go, source=None):
in_lib = partial_lib,
out_lib = out_lib,
objects = extra_objects,
archive = source.cgo_archive,
archives = source.cgo_archives,
)
data = GoArchiveData(
name = source.library.name,
Expand Down
7 changes: 3 additions & 4 deletions go/private/actions/pack.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def emit_pack(go,
in_lib = None,
out_lib = None,
objects = [],
archive = None):
archives = []):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update documentation in go/toolchains.rst#pack to reflect this change.

"""See go/toolchains.rst#pack for full documentation."""

if in_lib == None: fail("in_lib is a required parameter")
Expand All @@ -32,9 +32,8 @@ def emit_pack(go,
inputs.extend(objects)
arguments.add(objects, before_each="-obj")

if archive:
inputs.append(archive)
arguments.add(["-arc", archive])
inputs.extend(archives)
arguments.add(archives, before_each="-arc")

go.actions.run(
inputs = inputs,
Expand Down
10 changes: 8 additions & 2 deletions go/private/common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ hdr_exts = [

c_exts = [
".c",
".h",
]

cxx_exts = [
".cc",
".cxx",
".cpp",
Expand All @@ -56,7 +60,7 @@ cc_hdr_filetype = FileType(hdr_exts)

# Extensions of files we can build with the Go compiler or with cc_library.
# This is a subset of the extensions recognized by go/build.
cgo_filetype = FileType(go_exts + asm_exts + c_exts)
cgo_filetype = FileType(go_exts + asm_exts + c_exts + cxx_exts)

def pkg_dir(workspace_root, package_name):
"""Returns a relative path to a package directory from the root of the
Expand All @@ -75,12 +79,14 @@ def split_srcs(srcs):
asm = [],
headers = [],
c = [],
cxx = [],
)
ext_pairs = (
(sources.go, go_exts),
(sources.headers, hdr_exts),
(sources.asm, asm_exts),
(sources.c, c_exts),
(sources.cxx, cxx_exts),
)
extmap = {}
for outs, exts in ext_pairs:
Expand All @@ -97,7 +103,7 @@ def split_srcs(srcs):
return sources

def join_srcs(source):
return source.go + source.headers + source.asm + source.c
return source.go + source.headers + source.asm + source.c + source.cxx

def env_execute(ctx, arguments, environment = {}, **kwargs):
"""env_executes a command in a repository context. It prepends "env -i"
Expand Down
12 changes: 7 additions & 5 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ def _new_args(go):
"-compiler_path", go.cgo_tools.compiler_path,
"-cc", go.cgo_tools.compiler_executable,
])
args.add(go.cgo_tools.compiler_options, before_each = "-c_flag")
args.add(go.cgo_tools.compiler_options, before_each = "-cxx_flag")
args.add(go.cgo_tools.compiler_options, before_each = "-cpp_flag")
args.add(go.cgo_tools.linker_options, before_each = "-ld_flag")
return args
Expand Down Expand Up @@ -129,10 +131,10 @@ def _merge_embed(source, embed):
source["runfiles"] = source["runfiles"].merge(s.runfiles)
source["cgo_deps"] = source["cgo_deps"] + s.cgo_deps
source["cgo_exports"] = source["cgo_exports"] + s.cgo_exports
if s.cgo_archive:
if source["cgo_archive"]:
fail("multiple libraries with cgo_archive embedded")
source["cgo_archive"] = s.cgo_archive
if s.cgo_archives:
if source["cgo_archives"]:
fail("multiple libraries with cgo_archives embedded")
source["cgo_archives"] = s.cgo_archives

def _library_to_source(go, attr, library, coverage_instrumented):
#TODO: stop collapsing a depset in this line...
Expand All @@ -149,7 +151,7 @@ def _library_to_source(go, attr, library, coverage_instrumented):
"deps" : getattr(attr, "deps", []),
"gc_goopts" : getattr(attr, "gc_goopts", []),
"runfiles" : go._ctx.runfiles(collect_data = True),
"cgo_archive" : None,
"cgo_archives" : [],
"cgo_deps" : [],
"cgo_exports" : [],
}
Expand Down
70 changes: 57 additions & 13 deletions go/private/rules/cgo.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,23 @@ def _c_filter_options(options, blacklist):
return [opt for opt in options
if not any([opt.startswith(prefix) for prefix in blacklist])]

def _select_archive(files):
def _select_archives(files):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is safe. See the documentation below. cc_library may produce multiple output files, and we want exactly one of them.

Instead of calling this once on ctx.files.libs, call for each target, i.e., [_select_archive(lib.files) for lib in ctx.attr.libs]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still need to check that one out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok got it, i've pushed a fix

"""Selects a single archive from a list of files produced by a
static cc_library.

In some configurations, cc_library can produce multiple files, and the
order isn't guaranteed, so we can't simply pick the first one.
"""
# list of file extensions in descending order or preference.
outs = []
exts = [".pic.lo", ".lo", ".a"]
for ext in exts:
for f in files:
if f.basename.endswith(ext):
return f
fail("cc_library did not produce any files")
outs.append(f)
if not outs:
fail("cc_library did not produce any files")
return outs

def _cgo_codegen_impl(ctx):
go = go_context(ctx)
Expand All @@ -111,6 +114,7 @@ def _cgo_codegen_impl(ctx):
args.add(["-cc", str(cc), "-objdir", out_dir])

c_outs = [cgo_export_h, cgo_export_c]
cxx_outs = [cgo_export_h]
transformed_go_outs = []
gen_go_outs = [cgo_types]

Expand All @@ -135,6 +139,11 @@ def _cgo_codegen_impl(ctx):
gen_file = go.declare_file(go, path=mangled_stem + ".cgo1."+src_ext)
c_outs.append(gen_file)
args.add(["-src", gen_file.path + "=" + src.path])
for src in source.cxx:
mangled_stem, src_ext = _mangle(src, stems)
gen_file = go.declare_file(go, path=mangled_stem + ".cgo1."+src_ext)
cxx_outs.append(gen_file)
args.add(["-src", gen_file.path + "=" + src.path])

inputs = sets.union(ctx.files.srcs, go.crosstool, go.stdlib.files,
*[d.cc.transitive_headers for d in ctx.attr.deps])
Expand Down Expand Up @@ -164,7 +173,7 @@ def _cgo_codegen_impl(ctx):
args.add(copts)
ctx.actions.run(
inputs = inputs + go.crosstool,
outputs = c_outs + gen_go_outs + transformed_go_outs + [cgo_main],
outputs = c_outs + cxx_outs + gen_go_outs + transformed_go_outs + [cgo_main],
mnemonic = "CGoCodeGen",
progress_message = "CGoCodeGen %s" % ctx.label,
executable = go.builders.cgo,
Expand All @@ -185,6 +194,7 @@ def _cgo_codegen_impl(ctx):
),
OutputGroupInfo(
c_files = sets.union(c_outs, source.headers),
cxx_files = sets.union(cxx_outs, source.headers),
go_files = sets.union(transformed_go_outs, gen_go_outs),
main_c = as_set([cgo_main]),
),
Expand All @@ -199,6 +209,8 @@ _cgo_codegen = go_rule(
providers = ["cc"],
),
"copts": attr.string_list(),
"cxxopts": attr.string_list(),
"cppopts": attr.string_list(),
"linkopts": attr.string_list(),
},
)
Expand Down Expand Up @@ -254,7 +266,7 @@ def _not_pure(ctx, mode):
def _cgo_resolve_source(go, attr, source, merge):
library = source["library"]
cgo_info = library.cgo_info

source["orig_srcs"] = cgo_info.orig_srcs
source["runfiles"] = cgo_info.runfiles
if source["mode"].pure:
Expand All @@ -266,7 +278,7 @@ def _cgo_resolve_source(go, attr, source, merge):
source["cover"] = cgo_info.transformed_go_srcs
source["cgo_deps"] = cgo_info.cgo_deps
source["cgo_exports"] = cgo_info.cgo_exports
source["cgo_archive"] = cgo_info.cgo_archive
source["cgo_archives"] = cgo_info.cgo_archives

def _cgo_collect_info_impl(ctx):
go = go_context(ctx)
Expand All @@ -283,7 +295,7 @@ def _cgo_collect_info_impl(ctx):
gen_go_srcs = codegen.gen_go + import_files,
cgo_deps = codegen.deps,
cgo_exports = codegen.exports,
cgo_archive = _select_archive(ctx.files.lib),
cgo_archives = _select_archives(ctx.files.libs),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update documentation for GoSource in go/providers.rst to reflect this change.

FYI, I'm okay with small API breaking changes like this in providers between major versions. I doubt many people are using this yet. In the future, we may need to be more strict about this though.

runfiles = runfiles,
),
)
Expand All @@ -305,7 +317,7 @@ _cgo_collect_info = go_rule(
mandatory = True,
providers = [_CgoCodegen],
),
"lib": attr.label(
"libs": attr.label_list(
mandatory = True,
providers = ["cc"],
),
Expand All @@ -315,7 +327,7 @@ _cgo_collect_info = go_rule(
"""No-op rule that collects information from _cgo_codegen and cc_library
info into a GoSourceList provider for easy consumption."""

def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
def setup_cgo_library(name, srcs, cdeps, copts, cxxopts, cppopts, clinkopts):
# Apply build constraints to source files (both Go and C) but not to header
# files. Separate filtered Go and C sources.

Expand All @@ -325,13 +337,17 @@ def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
"external/" + REPOSITORY_NAME[1:] if len(REPOSITORY_NAME) > 1 else "",
PACKAGE_NAME)
copts = copts + ["-I", base_dir]
cxxopts = cxxopts + ["-I", base_dir]
cppopts = cppopts + ["-I", base_dir]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're separating copts and cppopts, this flag should only be added to cppopts.

Please also double-check all mentions of copts in this file. There are several places in _cgo_codegen_impl where it should be changed to cppopts. Basically anything that isn't specific to C or C++ should be changed.


cgo_codegen_name = name + ".cgo_codegen"
_cgo_codegen(
name = cgo_codegen_name,
srcs = srcs,
deps = cdeps,
copts = copts,
cxxopts = cxxopts,
cppopts = cppopts,
linkopts = clinkopts,
visibility = ["//visibility:private"],
)
Expand All @@ -352,6 +368,14 @@ def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
visibility = ["//visibility:private"],
)

select_cxx_files = name + ".select_cxx_files"
native.filegroup(
name = select_cxx_files,
srcs = [cgo_codegen_name],
output_group = "cxx_files",
visibility = ["//visibility:private"],
)

select_main_c = name + ".select_main_c"
native.filegroup(
name = select_main_c,
Expand All @@ -366,9 +390,9 @@ def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
platform_copts = _DEFAULT_PLATFORM_COPTS
platform_linkopts = platform_copts

cgo_lib_name = name + ".cgo_c_lib"
cgo_c_lib_name = name + ".cgo_c_lib"
native.cc_library(
name = cgo_lib_name,
name = cgo_c_lib_name,
srcs = [select_c_files],
deps = cdeps,
copts = copts + platform_copts + [
Expand All @@ -382,13 +406,33 @@ def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
visibility = ["//visibility:private"],
)

cgo_cxx_lib_name = name + ".cgo_cxx_lib"
native.cc_library(
name = cgo_cxx_lib_name,
srcs = [select_cxx_files],
deps = cdeps,
copts = cxxopts + platform_copts + [
# The generated thunks often contain unused variables.
"-Wno-unused-variable",
],
linkopts = clinkopts + platform_linkopts,
linkstatic = 1,
# _cgo_.o needs all symbols because _cgo_import needs to see them.
alwayslink = 1,
visibility = ["//visibility:private"],
)
cgo_libs = [
cgo_c_lib_name,
cgo_cxx_lib_name,
]

# Create a loadable object with no undefined references. cgo reads this
# when it generates _cgo_import.go.
cgo_o_name = name + "._cgo_.o"
native.cc_binary(
name = cgo_o_name,
srcs = [select_main_c],
deps = cdeps + [cgo_lib_name],
deps = cdeps + cgo_libs,
copts = copts,
linkopts = clinkopts,
visibility = ["//visibility:private"],
Expand All @@ -408,7 +452,7 @@ def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
name = cgo_embed_name,
srcs = srcs,
codegen = cgo_codegen_name,
lib = cgo_lib_name,
libs = cgo_libs,
cgo_import = cgo_import_name,
visibility = ["//visibility:private"],
)
Expand Down
2 changes: 2 additions & 0 deletions go/private/rules/wrappers.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ _CGO_ATTRS = {
"srcs": None,
"cdeps": [],
"copts": [],
"cxxopts": [],
"cppopts": [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update attribute tables in go/core.rst to include the new attributes.

This may break people that have C++ code that currently depends on options in copts. Hopefully this won't affect many people. I'll file an issue on Gazelle to separate these.

"clinkopts": [],
}

Expand Down
25 changes: 13 additions & 12 deletions go/tools/builders/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ type GoEnv struct {
goarch string
tags string
cc string
c_flags multiFlag
cxx_flags multiFlag
cpp_flags multiFlag
ld_flags multiFlag
shared bool
Expand Down Expand Up @@ -104,7 +106,9 @@ func envFlags(flags *flag.FlagSet) *GoEnv {
flags.StringVar(&env.tags, "tags", "", "Only pass through files that match these tags.")
flags.BoolVar(&env.shared, "shared", false, "Build in shared mode")
flags.StringVar(&env.cc, "cc", "", "Sets the c compiler to use")
flags.Var(&env.cpp_flags, "cpp_flag", "An entry to add to the c compiler flags")
flags.Var(&env.c_flags, "c_flag", "An entry to add to the C compiler flags")
flags.Var(&env.cxx_flags, "cxx_flag", "An entry to add to the C++ compiler flags")
flags.Var(&env.cpp_flags, "cpp_flag", "An entry to add to the CPP compiler flags")
flags.Var(&env.ld_flags, "ld_flag", "An entry to add to the c linker flags")
return env
}
Expand Down Expand Up @@ -150,18 +154,15 @@ func (env *GoEnv) env() []string {
fmt.Sprintf("CXX=%s", cc),
)
}
if len(env.cpp_flags) > 0 {
v := strings.Join(env.cpp_flags, " ")
result = append(result,
fmt.Sprintf("CGO_CFLAGS=%s", v),
fmt.Sprintf("CGO_CPPFLAGS=%s", v),
fmt.Sprintf("CGO_CXXFLAGS=%s", v),
)
cgoFlags := map[string]multiFlag{
"CGO_CFLAGS": env.c_flags,
"CGO_CXXFLAGS": env.cxx_flags,
"CGO_CPPFLAGS": env.cpp_flags,
"CGO_LDFLAGS": env.ld_flags,
}
if len(env.ld_flags) > 0 {
result = append(result,
fmt.Sprintf("CGO_LDFLAGS=%s", strings.Join(env.ld_flags, " ")),
)
for envVar, flags := range cgoFlags {
v := strings.Join(flags, " ")
result = append(result, envVar+"="+v)
}
return result
}
Expand Down
Loading