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

test: test for issue 30908 broke the noopt builder #31034

Closed
ALTree opened this issue Mar 25, 2019 · 7 comments
Closed

test: test for issue 30908 broke the noopt builder #31034

ALTree opened this issue Mar 25, 2019 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@ALTree
Copy link
Member

ALTree commented Mar 25, 2019

The new test introduced in CL 168957 (as 6582ee9) broke the noopt builder:

https://build.golang.org/log/e35b8d6bf5441c59c6b5f34c198c133b100f57e9

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 25, 2019
@ALTree ALTree added this to the Go1.13 milestone Mar 25, 2019
@ALTree
Copy link
Member Author

ALTree commented Mar 25, 2019

cc @thanm

@ALTree ALTree added the Testing An issue that has been verified to require only test changes, not just a test failure. label Mar 25, 2019
@thanm
Copy link
Contributor

thanm commented Mar 25, 2019

I can reproduce this fairly easily with toy programs. Ex:

go build -gcflags="os=-l -N" -ldflags="-strictdups=2" himom.go

Essentially what's happening here is that we have a set of functions that are being compiled in multiple places (ex: error.Error), and importantly both in the runtime and in code outside of the runtime.

On the no-opt builder, only the runtime is being being built with optimization, so the text symbol will have one length (and will have corresponding DWARF .debug_info section), and then when the same routine is compiled in no-opt mode, we'll see different length text (since optimization is off) and corresponding different .debug_info.

It's definitely useful to know about this problem, because it essentially says that programs built with a mix of optimization levels (some but not all packages built with "-l -N") are going to have DWARF that is at best suspect and at worst corrupted.

I will work on developing a fix.

The option that comes to mind would be to hash the contents of the .text section, and store it in the text symbol in some way (either as part of the symbol name, or stored in the symbol data in some fashion). The linker would then select a specific .text symbol, and once that is done we can include just the .debug_info (and related sections) that correspond to that version. Once we've determined which .text syms are being kept and which are being discarded, then we can do the sanity checking of the debug symbols then.

@thanm thanm self-assigned this Mar 25, 2019
@randall77
Copy link
Contributor

It sounds to me like the exception that allows the runtime to be compiled optimized even when compiling with -l -N, is broken. That exception shouldn't extend to error.Error, for example. (Or if it does, it should always, even when not compiling the runtime.)

@thanm
Copy link
Contributor

thanm commented Mar 25, 2019

That's an interesting idea -- so you're suggesting that the compiler honor "-l -N" for the various builtin functions while not honoring it for the rest of the runtime.

Another possibility would be to just whitelist these functions from the checking. It is a relatively small set, looks like

type..eq.[4]string<1>
type..eq.[6]string<1>
type..eq.[8]string<1>
type..hash.[4]string<1>
type..hash.[6]string<1>
type..hash.[8]string<1>
go.builtin.error.Error<1>

@randall77
Copy link
Contributor

That's an interesting idea -- so you're suggesting that the compiler honor "-l -N" for the various builtin functions while not honoring it for the rest of the runtime.

Right.
Or it could ignore "-l -N" for the builtin functions just as it does for the runtime. The important point is to be consistent about that choice.

@gopherbot
Copy link

Change https://golang.org/cl/169161 mentions this issue: test: fix fixedbugs/issue30908.go to work with no-opt builder

@gopherbot
Copy link

Change https://golang.org/cl/169160 mentions this issue: cmd/link: change -strictdups checking to handle mix of flags

gopherbot pushed a commit that referenced this issue Mar 27, 2019
Update the recently-added '-strictdups' sanity checking to avoid
failing the link in cases where we have objects feeding into the link
with a mix of command line flags (and in particular some with "-N" and
some without). This scenario will trigger errors/warnings due to
inlinable functions and wrapper functions that have different sizes
due to presence or lack of optimization.

Update #31034.

Change-Id: I1dd9e37c2f9bea5da0ab82e32e6fc210aebf6a65
Reviewed-on: https://go-review.googlesource.com/c/go/+/169160
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Mar 26, 2020
@rsc rsc unassigned thanm Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

4 participants