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.

Sign up for GitHub

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

Fix cgo build failures with specific GCC flags #2097

Merged
merged 3 commits into from
Jun 24, 2019
Merged

Fix cgo build failures with specific GCC flags #2097

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
457e4e7
Blacklist more cgo breaking warnings
AustinSchuh Jun 17, 2019
a2c8859
Remove unused variable warnings from _cgo_main.c compilation
AustinSchuh Jun 17, 2019
2da4b47
Support arbitrary -fmax-errors=
AustinSchuh Jun 18, 2019
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
2 changes: 2 additions & 0 deletions go/private/context.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ _GoContextData = provider()

_COMPILER_OPTIONS_BLACKLIST = {
"-fcolor-diagnostics": None,
"-fdiagnostics-color": None,
"-fmax-errors=20": None,
AustinSchuh marked this conversation as resolved.
Show resolved Hide resolved
"-Wall": None,

# Symbols are needed by Go, so keep them
Expand Down
5 changes: 4 additions & 1 deletion go/private/rules/cgo.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,10 @@ def setup_cgo_library_for_mode(name, srcs, cdeps, copts, cxxopts, cppopts, clink
name = cgo_o_name,
srcs = [select_main_c],
deps = cdeps + cgo_o_deps,
copts = copts + cppopts,
copts = copts + cppopts + [
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is deprecated and is only used to build cgo code for a very narrow legacy case.

_cgo_context_data_impl is probably the right place to add this flag (to c_compile_options).

Copy link
Contributor Author

@AustinSchuh AustinSchuh Jun 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've confirmed by A/B building with and without this line that this is the exact target that is causing the problem.

I'm building github.com/mattn/go-sqlite3

The build rule generated is:

go_library(
    name = "go_default_library",
    srcs = [ 
        "backup.go",
        "callback.go",
        "doc.go",
        "error.go",
        "sqlite3.go",
        "sqlite3-binding.c",
        "sqlite3-binding.h",
        "sqlite3_context.go",
        "sqlite3_go18.go",
        "sqlite3_load_extension.go",
        "sqlite3_other.go",
        "sqlite3_solaris.go",
        "sqlite3_type.go",
        "sqlite3_windows.go",
        "sqlite3ext.h",
        "static_mock.go",
    ],  
    cgo = True,
    clinkopts = select({
        "@io_bazel_rules_go//go/platform:linux": [
            "-ldl",
        ],
        "@io_bazel_rules_go//go/platform:solaris": [
            "-lc",
        ],
        "@io_bazel_rules_go//go/platform:windows": [
            "-lmingwex -lmingw32",
        ],
        "//conditions:default": [], 
    }), 
    copts = [ 
        "-std=gnu99",
        "-DSQLITE_ENABLE_RTREE -DSQLITE_THREADSAFE=1 -DHAVE_USLEEP=1",
        "-DSQLITE_ENABLE_FTS3 -DSQLITE_ENABLE_FTS3_PARENTHESIS -DSQLITE_ENABLE_FTS4_UNICODE61",
        "-DSQLITE_TRACE_SIZE_LIMIT=15",
        "-DSQLITE_DISABLE_INTRINSIC",
        "-Wno-deprecated-declarations",
    ] + select({
        "@io_bazel_rules_go//go/platform:android": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:darwin": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:dragonfly": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:freebsd": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:linux": [
            "-DHAVE_PREAD64=1 -DHAVE_PWRITE64=1",
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:nacl": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:netbsd": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:openbsd": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:plan9": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:solaris": [
            "-D__EXTENSIONS__=1",
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:windows": [
            "-I. -fno-stack-check -fno-stack-protector -mno-stack-arg-probe",
        ],
        "//conditions:default": [], 
    }) + select({
        "@io_bazel_rules_go//go/platform:windows_386": [
            "-D_USE_32BIT_TIME_T",
        ],
        "//conditions:default": [], 
    }), 
    importpath = "github.com/mattn/go-sqlite3",
    visibility = ["//visibility:public"],
)

I put it here since restricting it to the smallest scope seemed like a good idea rather than being more broad. I'm happy to do whatever you want though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's strange. Is that on master or an older version of rules_go? I tried bazel build @com_github_mattn_go_sqlite3//:go_default_library with rules_go and go-sqlite3 at master, but setup_cgo_library_for_mode wasn't called (checked by adding a fail in there).

By any chance, do you have objc = True set in a rule somewhere?

In any case please do add this to _cgo_context_data_impl. The legacy path should disappear at some point soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for you, I upgraded to the latest released rules_go (0.18.6), and I'm still seeing it. I'm using Gazelle 0.17.0. The only objc = True in our code base is in a test in rules_go.

I removed the offending rule and gazelle added it back without changes. So I think it's not some legacy flag on the rule. I suspect Bazel is building the binary even if it is unused for completeness.

I used -s with bazel and changed some cgo code to add an unused variable warning and already found -Wno-unused-variable in the command line used to build the cgo generated code, and in the command line used to build standalone c code. I'm reluctant to add it somewhere else since that would duplicate the flag.

Let me know what you want to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the release-0.18 branch was cut before the new cgo stuff was merged, so this path will still be active there. It shouldn't be active at master though (unless objc = True).

So I think this option should in both _cgo_context_data_impl and setup_cgo_library_for_mode. I'll backport the latter part to release-0.18 after this is merged (the next minor release will likely be early July).

Some time after release-0.19 is cut, I'll remove the legacy path, which will un-duplicate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your request got hard... I want to test master with the failing code before sending it out for review, but that's a fair amount of work. I'm tempted to just roll this part back and revisit when you cut a new release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you. I'm fine with this being left here, just be aware that the old path will disappear in a few weeks.

Copy link
Contributor Author

@AustinSchuh AustinSchuh Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Let's just do that then. Merge this as is (assuming you have no more feedback), and then when we upgrade again, I'll take stock of how things look and send you any reviews needed to finish this.

Thanks Jay!

# The generated thunks often contain unused variables.
"-Wno-unused-variable",
],
linkopts = clinkopts,
visibility = ["//visibility:private"],
**common_attrs
Expand Down