Fix inlining failures when using bazel#12
Conversation
|
@yuzefovich @jordanlewis please take a look |
yuzefovich
left a comment
There was a problem hiding this comment.
Some of the inline statements do not appear in the compiler output when the go compiler is invoked through bazel.
Can you say a bit more about this? Does it mean that direct go invocation produces lines like was inlined for some functions whereas bazel invocation doesn't?
gcassert_test.go
Outdated
| func TestGCAssert(t *testing.T) { | ||
| var w strings.Builder | ||
| err := GCAssert(&w, false /* useBazel */, "./testdata", "./testdata/otherpkg") | ||
| err := GCAssert(&w, false /* useBazel */, false /* debug*/, "./testdata", "./testdata/otherpkg") |
There was a problem hiding this comment.
nit: add a space after debug.
nit: also update README.md with the new arg.
bazel_utils.go
Outdated
| return exec.Command("bazel", args...) | ||
| } | ||
|
|
||
| func getBazelBin() (string, error) { |
There was a problem hiding this comment.
nit: getBazelBin and getWorkspacePath are almost the same, maybe extract a helper.
bazel_utils.go
Outdated
| mvcc.go:1135 0x66928a 94000000 CALL 0(PC) [0:4]R_CALLARM64:github.com/cockroachdb/cockroach/pkg/storage.(*MVCCGetOptions).validate | ||
|
|
||
| */ | ||
| callRegex, err := regexp.Compile(`\s*(\S+)\s*\S*\s*\S*\s*(CALL)\s*\S*\s*(\S+)`) |
There was a problem hiding this comment.
nit: we could remove the capturing group for (i.e. the parenthesis around) CALL characters since we don't actually need to capture it, up to you though.
bazel_utils.go
Outdated
| if fnIdentifierMatch != nil { | ||
| // See example call above for details on why this manipulation works. | ||
| currentFn := fnIdentifierMatch[3][strings.LastIndex(fnIdentifierMatch[3], ":")+1:] | ||
| for _, inlineFn := range bazelInlineSites { |
There was a problem hiding this comment.
nit: not sure if this is important, but we could make bazelInlineSites a map to speed up this search.
5d97194 to
ecab5e4
Compare
Exactly, for some function calls we do not get |
bazel_utils.go
Outdated
|
|
||
| if !atleastMatchedOnce { | ||
| errorMsg := `callRegex did not match any line in the disassembled object code. | ||
| This means it did not find any non-inlined function calls in the inspected object files. Ensure that the regex is still correct if this |
There was a problem hiding this comment.
nit: we're not in crdb repo (where this is mandated in the style guide), but there are a few lines and comments (as well as in the other file) that are too long, consider wrapping them.
|
Oops, just realized that a couple of comments that I had in response to your previous comments are now removed because I marked them as "resolved" (I'm not used to using the github review modal :/). (Trying to reproduce those.)
|
Some of the inline statements do not appear in the compiler output when the go compiler is invoked through bazel. This code change makes gcassert inspect the object files directly to check if a function call that must always be inlined was ever not inlined (when using bazel).
ecab5e4 to
819f79a
Compare
|
I wrapped a few long lines and addressed the other nits. I will add the new runbook step in cockroachdb/cockroach#103471. Thanks for the quick review! |
|
Will defer to Yahor for the review, please keep in mind that this tool should remain usable outside of Bazel as well. |
|
Ahmad effectively implemented a parallel path for bazel builds, so the existing non-bazel one should continue working. |
…azel Fix inlining failures when using bazel
…azel Fix inlining failures when using bazel
…azel Fix inlining failures when using bazel
…azel Fix inlining failures when using bazel
…azel Fix inlining failures when using bazel
This code change pulls changes from jordanlewis/gcassert#11 and jordanlewis/gcassert#12 to let gcassert use bazel to build if the `--use-bazel` flag is set. It now checks if inline directives passed by inspecting object files because the compiler output is missing some inline statements when invoked through `bazel`. Release note: None Epic: CRDB-8349 Closes cockroachdb#65485
Some of the inline statements do not appear in the compiler output when the go compiler is invoked through bazel.
This code change makes gcassert inspect the object files directly to check if a function call that must always be inlined was ever not inlined (when using bazel).