-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Proposal: Remove debug symbols from build #17678
Conversation
changelog/17678.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:improvement | |||
Remove debug symbols from release binaries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the writeup, we expect the primary externally-visible change to be "reduced binary size", and I think that might be a better way of describing the change in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This is a great writeup @swenson and addresses my primary concern but I'm going to defer actual reviewing duties to the other reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your reasoning, this is a great improvement 👍
@@ -258,7 +258,7 @@ ci-verify: | |||
# This is used for release builds by .github/workflows/build.yml | |||
build: | |||
@echo "--> Building Vault $(VAULT_VERSION)" | |||
@go build -v -tags "$(GO_TAGS)" -ldflags " -X github.com/hashicorp/vault/sdk/version.Version=$(VAULT_VERSION) -X github.com/hashicorp/vault/sdk/version.GitCommit=$(VAULT_REVISION) -X github.com/hashicorp/vault/sdk/version.BuildDate=$(VAULT_BUILD_DATE)" -o dist/ | |||
@go build -v -tags "$(GO_TAGS)" -ldflags " -s -w -X github.com/hashicorp/vault/sdk/version.Version=$(VAULT_VERSION) -X github.com/hashicorp/vault/sdk/version.GitCommit=$(VAULT_REVISION) -X github.com/hashicorp/vault/sdk/version.BuildDate=$(VAULT_BUILD_DATE)" -o dist/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think technically the -w
is redundant, as -s
is a superset. In my testing both -s
and -s -w
produced byte-for-byte identical results on a different package, but no harm in keeping it. [ref]
-s
Omit the symbol table and debug information.
-w
Omit the DWARF symbol table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this in my testing as well. All of the recommendations I've found say to use both, so I did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the +1s. Any thoughts @jasonodonnell @ncabatoff @sgmiller? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern was would this prevent us from using pprof
, but that doesn't seem to be the case, so 👍.
By adding the link flags `-s -w` we can reduce the Vault binary size from 204 MB to 167 MB (about 18% reduction in size). This removes the DWARF section of the binary. i.e., before: ``` $ objdump --section-headers vault-debug vault-debug: file format mach-o arm64 Sections: Idx Name Size VMA Type 0 __text 03a00340 0000000100001000 TEXT 1 __symbol_stub1 00000618 0000000103a01340 TEXT 2 __rodata 00c18088 0000000103a01960 DATA 3 __rodata 015aee18 000000010461c000 DATA 4 __typelink 0004616c 0000000105bcae20 DATA 5 __itablink 0000eb68 0000000105c10fa0 DATA 6 __gosymtab 00000000 0000000105c1fb08 DATA 7 __gopclntab 02a5b8e0 0000000105c1fb20 DATA 8 __go_buildinfo 00008c10 000000010867c000 DATA 9 __nl_symbol_ptr 00000410 0000000108684c10 DATA 10 __noptrdata 000fed00 0000000108685020 DATA 11 __data 0004e1f0 0000000108783d20 DATA 12 __bss 00052520 00000001087d1f20 BSS 13 __noptrbss 000151b0 0000000108824440 BSS 14 __zdebug_abbrev 00000129 000000010883c000 DATA, DEBUG 15 __zdebug_line 00651374 000000010883c129 DATA, DEBUG 16 __zdebug_frame 001e1de9 0000000108e8d49d DATA, DEBUG 17 __debug_gdb_scri 00000043 000000010906f286 DATA, DEBUG 18 __zdebug_info 00de2c09 000000010906f2c9 DATA, DEBUG 19 __zdebug_loc 00a619ea 0000000109e51ed2 DATA, DEBUG 20 __zdebug_ranges 001e94a6 000000010a8b38bc DATA, DEBUG ``` And after: ``` $ objdump --section-headers vault-no-debug vault-no-debug: file format mach-o arm64 Sections: Idx Name Size VMA Type 0 __text 03a00340 0000000100001000 TEXT 1 __symbol_stub1 00000618 0000000103a01340 TEXT 2 __rodata 00c18088 0000000103a01960 DATA 3 __rodata 015aee18 000000010461c000 DATA 4 __typelink 0004616c 0000000105bcae20 DATA 5 __itablink 0000eb68 0000000105c10fa0 DATA 6 __gosymtab 00000000 0000000105c1fb08 DATA 7 __gopclntab 02a5b8e0 0000000105c1fb20 DATA 8 __go_buildinfo 00008c20 000000010867c000 DATA 9 __nl_symbol_ptr 00000410 0000000108684c20 DATA 10 __noptrdata 000fed00 0000000108685040 DATA 11 __data 0004e1f0 0000000108783d40 DATA 12 __bss 00052520 00000001087d1f40 BSS 13 __noptrbss 000151b0 0000000108824460 BSS ``` The only side effect I have been able to find is that it is no longer possible to use [delve](https://github.com/go-delve/delve) to run the Vault binary. Note, however, that running delve and other debuggers requires access to the full source code, which isn't provided for the Enterprise, HSM, etc. binaries, so it isn't possible to debug those anyway outside of people who have the full source. * panic traces * `vault debug` * error messages * Despite what the documentation says, these flags do *not* delete the function symbol table (so it is not the same as having a `strip`ped binary). It contains mappings between the compiled binary and functions, paramters, and variables in the source code. Using `llvm-dwarfdump`, it looks like: ``` 0x011a6d85: DW_TAG_subprogram DW_AT_name ("github.com/hashicorp/vault/api.(*replicationStateStore).recordState") DW_AT_low_pc (0x0000000000a99300) DW_AT_high_pc (0x0000000000a99419) DW_AT_frame_base (DW_OP_call_frame_cfa) DW_AT_decl_file ("/home/swenson/vault/api/client.go") DW_AT_external (0x01) 0x011a6de1: DW_TAG_formal_parameter DW_AT_name ("w") DW_AT_variable_parameter (0x00) DW_AT_decl_line (1735) DW_AT_type (0x00000000001e834a "github.com/hashicorp/vault/api.replicationStateStore *") DW_AT_location (0x009e832a: [0x0000000000a99300, 0x0000000000a9933a): DW_OP_reg0 RAX [0x0000000000a9933a, 0x0000000000a99419): DW_OP_call_frame_cfa) 0x011a6def: DW_TAG_formal_parameter DW_AT_name ("resp") DW_AT_variable_parameter (0x00) DW_AT_decl_line (1735) DW_AT_type (0x00000000001e82a2 "github.com/hashicorp/vault/api.Response *") DW_AT_location (0x009e8370: [0x0000000000a99300, 0x0000000000a9933a): DW_OP_reg3 RBX [0x0000000000a9933a, 0x0000000000a99419): DW_OP_fbreg +8) 0x011a6e00: DW_TAG_variable DW_AT_name ("newState") DW_AT_decl_line (1738) DW_AT_type (0x0000000000119f32 "string") DW_AT_location (0x009e83b7: [0x0000000000a99385, 0x0000000000a99385): DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_piece 0x8 [0x0000000000a99385, 0x0000000000a993a4): DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8 [0x0000000000a993a4, 0x0000000000a993a7): DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8) ``` This says that the particular binary section is the function `github.com/hashicorp/vault/api.(*replicationStateStore).recordState`, from the file `/home/swenson/vault/api/client.go`, containing the `w` parameter on line 1735 mapped to certain registers and memory, the `resp` paramter on line 1735 mapped to certain reigsters and memory, and the `newState` variable on line 1738, mapped to certain registers, and memory. It's really only useful for a debugger. Anyone running the code in a debugger will need full access the source code anyway, so presumably they will be able to run `make dev` and build the version with the DWARF sections intact, and then run their debugger.
f066395
to
70cc570
Compare
Okay, with no further objections... Thanks everyone! |
This change was reverted with fc9dfa2 Generally the above change, and this change, would have been slightly bad for Linux distributions that packages vault. Generally would recommend to keep a distinction between "release builds" and "development builds". |
@Foxboron not sure what you mean. We don't provide development builds, only release builds, so it should not be expected to have all of the DWARF debug information. |
@swenson I actually somehow missed it, but you do provide development builds with the new Why would |
@Foxboron (The change was not reverted, btw -- I just confirmed, and we still pass
The release process uses Go debug binaries are not generally useful unless you have the complete source code associated with a build, and are only useful for stepping through with a debugger. So, presumably, if a user has gotten to that point of troubleshooting an issue, they'll have a development environment and are able to run Passing |
Right, I missed that there was two scripts building binaries. Sorry.
Arch Linux would provide complete debug packages for
Mm, I'm not aware of any issues running |
For debug packages for Arch, I'd probably recommend building them with I assumed It is most beneficial to use |
Which sections doesn't strip remove? It should remove everything.
|
At least on macOS, they are very different:
A quick look at |
(I suspect this is Apple's |
I don't know how golang nor I don't understand how Ubuntu would be different either.... |
Proposal: Remove debug symbols from release builds
By adding the link flags
-s -w
we can reduce the Vault binary size from 204 MB to 167 MB (about 18% reduction in size).What does this doe?
This removes the DWARF section of the binary.
i.e., before:
And after:
What does this affect?
The only side effect I have been able to find is that it is no longer possible to use delve to run the Vault release binary.
Note, however, that running delve and other debuggers requires access to the full source code, which isn't provided for the Enterprise, HSM, etc. binaries, so it isn't possible to debug those anyway outside of people who have the full source.
What does this not affect?
vault debug
strip
ped binary).Okay, but what is in the removed sections?
The DWARF sections contain mappings between the compiled binary and functions, parameters, and variables in the source code.
Using
llvm-dwarfdump
, it looks like:This says that the particular binary section is the function
github.com/hashicorp/vault/api.(*replicationStateStore).recordState
, from the file/home/swenson/vault/api/client.go
, containing thew
parameter on line 1735 mapped to certain registers and memory, theresp
parameter on line 1735 mapped to certain registers and memory, and thenewState
variable on line 1738, mapped to certain registers, and memory.It's really only useful for a debugger.
You can see some details in the
debug/dwarf
docs.What if I need to run in a debugger?
Anyone running the code in a debugger will need full access the source code anyway, so presumably they will be able to run
make dev
and build the version with the DWARF sections intact, and then run their debugger.Initially I had proposed releasing a separate
+debug
binary, but I've realized that I don't think this would actually be useful to anyone?