build: bazel-generated go binaries truncate the path of stored source file names

[knz]

Compare running the following command:

cockroach demo --empty -e " select crdb_internal.force_error('XX000', 'woo')"

Using either a binary produced by bazel build, or one produced by make buildshort.

With make buildshort, the stack trace looks like this:

DETAIL: stack trace:
github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtins.go:4082: func223()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/eval.go:4049: Eval()
github.com/cockroachdb/cockroach/pkg/sql/values.go:156: startExec()
...

With bazel, the stack trace looks like this:

DETAIL: stack trace:
pkg/sql/sem/builtins/builtins.go:4082: func223()
pkg/sql/sem/tree/eval.go:4049: Eval()
pkg/sql/values.go:156: startExec()

The 2nd one is incorrect.

[knz]

Investigation:

• I was able to confirm that the .a files generated for each sub-package properly include the pkg/ prefix in the object definitions therein.
• the link script in bazel-out/freebsd-fastbuild/bin/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short-0.params contain all the paths with a pkg/ prefix, so that's also fine.

However I see that the final link stage is not done using the go executable, but instead a program called builder, in bazel-out/host/bin/external/go_sdk/builder.

@rickystewart informs me that this program comes from https://github.com/bazelbuild/rules_go/tree/master/go/tools/builders?rgh-link-date=2021-04-28T17%3A07%3A05Z

I will investigate this next.

[knz]

The previous investigation step might be mistaken btw -- I confirmed the files contain a pkg/ prefix, but I did not confirm whether they should also include the github package path. I will also compare the .a files produced for a make build to compare.

[knz]

So I looked at these .a files again.

The bazel-generated one has file..pkg/util/log/clog.go inside the object code.

The make-generated one has file../data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/util/log/clog.go

So that means the .a generation is one place where there's something wrong going on already.

[knz]

Ok so the low level build command is this:

bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix freebsd_amd64 -tags bazel,crdb_test_off,bazel,crdb_test_off \
  -src pkg/util/log/ambient_context.go
  -src pkg/util/log/channels.go
  -src pkg/util/log/clog.go
  -src pkg/util/log/doc.go
  ...

This suggests that the builer program doesn't know that these .go files are really homed in github.com/cockroachdb/cockroach, and doesn't insert this file prefix into the resulting object code.

We have two courses of action I think:

• either modify the bazel build files so that the source files are positioned in a subdirectory named github.com/cockroachdb/cockroach; or
• find out if there's a way to inject a path prefix in the file name strings generated by the builder program.

[knz]

@rickystewart what do you reckon is easier?

[knz]

builder seems to have an -importpath flag, I'll try that.

[knz]

Ok so a little bit more of sleuthing: the stack traces pull two different strings from object code to construct stack traces:

• the symbol path
• the file path

In the issue at top, the stack traces are incorrect due to incorrect file path. The symbol paths are OK.

The builder -importpath is already populated by the bazel rules, and properly changes the prefix of symbol paths. However, it has no effect on the embedded file paths.

[knz]

I then looked into how the file paths look like for different files in the repo.

For vendored dependencies

• via make, file name /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/vendor/github.com/cockroachdb/errors/errutil/as.go
• via bazel, file name external/com_github_cockroachdb_errors/errutil/as.go

For files in a package

• via make, file name /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/cli/cli.go
• via bazel, file name cli.go (not even a directory prefix)

For the main function

• via make, file name /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/cmd/cockroach-short/main.go
• via bazel, file name pkg/cmd/cockroach-short/main.go

There are two things that pop to the eye:

• the original issue at top: none of the bazel-generated files contain the package prefix
• a perhaps different issue: in some cases, even the pkg path is truncated entirely (see cli.go above)

[knz]

I think we could perhaps edit various parts of the code that inspect the file prefixes (from memory, that would be sentry-go, logging and error handling) however the case where even the package prefix is stripped entirely would be too limiting.

[knz]

Ok I found what happened with that cli.go.
It's possibly a bug in the go builder program. Look at these two lines:

https://github.com/bazelbuild/rules_go/blob/master/go/tools/builders/compilepkg.go#L280

https://github.com/bazelbuild/rules_go/blob/master/go/tools/builders/compilepkg.go#L287

(The -trimpath flag was introduced in go 1.13 and truncates the given path prefix from the file names embedded in objects)

The problem is that the "-trimpath=" + srcPath in the cgo case above is incorrect. It causes any file name X/Y/Z.go to be truncated to just Z.go when X/Y happens to contain Cgo definitions (which the cli package does).

It seems incorrect that the truncation would be different depending on whether or not Cgo is being used.

[knz]

Here are a couple of possible "next steps" from here:

1. double down on trying to address this issue, to populate the file path properly.
2. investigate the crdb source code and its dependencies to reduce the need for accurate file paths.

I'm not sure how much work the 2nd avenue is.

• For sentry-go, which was my primary concern, I think we're OK because the code uses the symbol path to determine the go package, not the file path.
• However, the various "stack trace formatting" functions we and our dependencies have (pkg/errors, cockroachdb/errors) usually exploit the file field, not the module field. That's because they use runtime.FuncForPC to get details, not runtime.CallerFrames. I don't know how much a lift it would be to change this. (Filed Stop relying on pkg/errors to handle stack traces errors#70 to track)

[knz]

I'm going to file yet another issue for that investigation, because it's hairy

[knz]

filed #64383

[rickystewart]

@rickystewart what do you reckon is easier?

The second (finding a way to inject the appropriate source path into the builder program) seems more elegant and maintainable, and upstreaming it seems more likely as well. Having said that, I don't know enough about how the Go toolchain works to speak to how that would actually be implemented, or whether it's possible in principle. The first is hackier, and it seems more dubious that we would get it right, though I suppose that change could be made if it had to. Having looked closer at this, I am less confident that option 1 is possible (see comment below).

The cli.go thing does indeed seem like a bug to me, i imagine we should fix that and get it upstreamed as well. I'll look closer at that today.

Here are a couple of possible "next steps" from here:

If repairing the problem from the Bazel side is possible, I'd prefer to do that -- seems perfectly likely that the fixes we make will positively impact the rest of the community as well, instead of requiring us to rewrite a bunch of our own code for no immediate benefit. In principle, I still don't disagree with the spirit of this sentence, but as I've looked closer at the Bazel end of things I do wonder if it wouldn't just be easier overall to just audit our source and make the requisite changes accordingly.

[rickystewart]

I then looked into how the file paths look like for different files in the repo.

...

I have a couple questions about this analysis.

1. How are you determining the filenames? Is there a command I can run from the shell given a .a file to get the stored filename?
2. I notice that the filenames stored are absolute paths (not starting with github.com/, as you see in the stack trace in the original comment). Does that happen later during linkage, or do we strip out the prefix when we're printing the stack trace to the screen (or something else)?

[rickystewart]

Ok so the low level build command is this:

bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix freebsd_amd64 -tags bazel,crdb_test_off,bazel,crdb_test_off \
  -src pkg/util/log/ambient_context.go
  -src pkg/util/log/channels.go
  -src pkg/util/log/clog.go
  -src pkg/util/log/doc.go
  ...

I looked at this -- these paths are relative to the execroot, i.e., a Bazel-owned path that looks like /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach. We are NOT in control of the directory layout of the execroot and related directories -- Bazel mandates that for us, and the execroot directory layout deliberately mirrors that of the normal workspace, i.e., it doesn't have a github.com/cockroachdb/cockroach prefix.

It's conceivable we could hack rules_go to fix this without interfering with any of Bazel's internal stuff, by staging all the sources in a temporary directory with the prefix? That's so ugly that it strikes me more as a worst-case workaround, and handling any corner cases that we may not be aware of at this point including vendored code will also be problematic.

[knz]

How are you determining the filenames? Is there a command I can run from the shell given a .a file to get the stored filename?

I ran ar x to extract the files inside, then strings _go_.o. Then search for .go filenames, or the file.. prefix. The difference in the output of string is visually striking anyway.

I notice that the filenames stored are absolute paths (not starting with github.com/, as you see in the stack trace in the original comment). Does that happen later during linkage, or do we strip out the prefix when we're printing the stack trace to the screen (or something else)?

• In most occasions (asking the Go runtime for a stack trace, panic objects etc), we will be seeing absolute paths.

• However, the cockroachdb/errors library and sentry-go will also strip the prefix in some cases, as determined by the go/build's package SrcDirs() function. That run-time trimming causes the output that I pasted at the top of this issue.

  This particular prefix trimming behavior is not essential for correctness, but is a good quality of life improvement.

  (Sentry-go needs a correct symbol path for correctness, which it already gets thanks to the -importpath arg, but we also benefit from a correct file path when processing sentry reports because it makes them more compact.)

• There is another, unrelated, run-time prefix trimming occurring in our custom callers package, which is used to generate location information in logging output. This is based off a regular expression that ignores the absolute source prefix, but wants to see the relative path within the source tree.

  This one we should get right (see further discussion below) for logging output: the callers package should see a relative file name so that we can distinguish logging output from files that have the same base name but are stored in different directories, as discussed in *: properly handle file path != package path when looking at stack traces #64383.

We are NOT in control of the directory layout of the execroot and related directories -- Bazel mandates that for us, and the execroot directory layout deliberately mirrors that of the normal workspace, i.e., it doesn't have a github.com/cockroachdb/cockroach prefix.

Yes, I understand that -- in the same way, we do not control where each member of the crdb team places their $GOPATH root directory. But that is not what matters here. What we want is to see the package path as suffix (not prefix) in the build.

In your example, we would benefit from the source files being copied inside each sandbox inside a path named like this:

/private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/github.com/cockroachdb/cockroach/pkg/cli
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sandbox prefix
                                                      (in-sandbox relative path)  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

With this path structure, we can use -trimpath=. inside the sandbox and still get the correct behavior.

There is an alternative approach, which I find a little bit icky but would be simple and would get us 80% of the way wrt ergonomics: modify callers and the other packages to construct a fake file path by concatenating the symbol path with the file base name. It wouldn't be correct for renamed packages, but since we have a 1-to-1 mapping inside crdb between absolute paths and package paths currently, it may be sufficient. Unsure.

[rickystewart]

The problem is that the "-trimpath=" + srcPath in the cgo case above is incorrect. It causes any file name X/Y/Z.go to be truncated to just Z.go when X/Y happens to contain Cgo definitions (which the cli package does).

It seems incorrect that the truncation would be different depending on whether or not Cgo is being used.

I looked at this. If you replace the -trimpath in the cgo case with -trimpath=. as in the other case, then the gofile looks like this instead:

gofile../var/folders/tk/7wfswg457lqbj05wtbm4ldpm0000gp/T/rules_go_work-379351293/cli.go

i.e., cli.go is staged in a temporary directory by the builder. cgo requires the .go source files to be preprocessed into a set of C and Go source files, which is presumably what's happening under the hood. The reason that the -trimpath arguments are different is that the cgo case is materially different from the non-cgo case -- the cgo files are staged in a temporary directory, i.e., not even in the bazel sandbox.

Yes, I understand that -- in the same way, we do not control where each member of the crdb team places their $GOPATH root directory. But that is not what matters here. What we want is to see the package path as suffix (not prefix) in the build.

I think you misunderstood me. My point isn't that we can't choose where the sandbox goes. (We actually can! The documentation I linked describes how to do that.) My point is that the inner structure of the sandbox directories is dictated by Bazel, and the sandbox mirrors the structure of the actual workspace.

So, can we make it so the files get copied into a subdirectory in the sandbox called github.com/cockroachdb/cockroach? Yes, of course we can -- I just have to mkdir -p github.com/cockroachdb/cockroach && mv pkg github.com/cockroachdb/cockroach/pkg && git add -A && git commit. Of course, that solution makes no sense, so it's a non-starter.

If the Bazel directory structure is insufficient, then we have the option of staging the source outside of the sandbox in a temporary directory, as in the cgo case described above. We can trivially choose what the directory structure of a temporary directory looks like. Of course, at this point, we're basically ignoring the Bazel sandbox and staging our own sandbox with a slightly different directory structure at additional complexity and I/O cost for what seems to me to be a very dubious benefit. I also don't know that that patch would be accepted upstream, and then I worry about how we're going to handle merge conflicts, etc.

I'll also bump #64383 to see how I can help out there.

[knz]

cli.go is staged in a temporary directory by the builder.

Can we make that temporary directory named after the same structure as the source code directory?

After all, that is what the go compiler must be doing, otherwise a plain go build would also have broken filenames.

[knz]

Something that wasn't really clarified so far is that today all engineers use text editors which have special support for parsing error messages, stack traces etc so that they can click a source file name and have the editor open it automatically.

This uses some form of fuzzy match because often the file name is truncated to a relative path, so the editor knows about a "project root directory" and resolves path names from there.

If the bazel sandbox completely shuffles the layout of files during compilation, we're removing that UX benefit.

[knz]

Given that bazel is also used for C++ code, and C++ code editors also do this navigation-via-file-name-in-error-messages, I wonder how it works in that case? Aren't C++ files located inside the sandbox using the same directory structure as in the source directory?

[rickystewart]

Can we make that temporary directory named after the same structure as the source code directory?

Presumably yes, with a patch. That fixes the cgo case, but not the non-cgo case. Does that make our lives significantly easier, or allow us to avoid or simplify some of the #64383 work? If so, I can look at changing how that temporary directory is staged.

[knz]

For the non-cgo case, I think we're somewhat OK for the code editor UX: as determined above alrady, the remaining paths are already stored as pkg/..., which are relative to the cockroach root repo. I think the text editors will be able to deal with this.

(If not, we can apply the same solution as for cgo and move the sources to a github/cockroachdb/cockroach sub-directory)

We wouldn't be able to auto-navigate easily to the dependency package sources since their path inside the sandbox is being snake_cased_like_this/foo.go. I don't know if that's an issue, but it seems less important.

[rickystewart]

Given that bazel is also used for C++ code, and C++ code editors also do this navigation-via-file-name-in-error-messages, I wonder how it works in that case? Aren't C++ files located inside the sandbox using the same directory structure as in the source directory?

Yes, same as (non-cgo) .go files.

We wouldn't be able to auto-navigate easily to the dependency package sources since their path inside the sandbox is being snake_cased_like_this/foo.go. I don't know if that's an issue, but it seems less important.

We're moving towards rm -rf vendor anyway, so we would require a different solution for this sort of thing anyway. If this becomes important we can keep an eye on it and adopt some tooling that the IDE's can get along with.

For the non-cgo case, I think we're somewhat OK for the code editor UX: as determined above alrady, the remaining paths are already stored as pkg/..., which are relative to the cockroach root repo. I think the text editors will be able to deal with this.

I agree. OK, I'll check on this and see what I can do for the cgo case.

[knz]

we would require a different solution for this sort of thing anyway

Well in the ideal world, we'd organize the directories inside the sandbox as follows:

• github.com/cockroachdb/cockroach/pkg/... for our sources
• github.com/some/dep/... for what was previously at vendor/github.com/some/dep/...

then all the paths would align properly and there wouldn't be any ambiguity

[RaduBerinde]

After spelunking through the compile code, I realized that -trimpath actually supports more general rewrite rules, like -trimpath=/a/b/c=>github.com/x/y. I think that might be a cleaner way to solve this problem.