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

cmd/link/internal/ld: abstract origin missing local variable #25459

Closed
FiloSottile opened this issue May 18, 2018 · 6 comments
Closed

cmd/link/internal/ld: abstract origin missing local variable #25459

FiloSottile opened this issue May 18, 2018 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@FiloSottile
Copy link
Contributor

0xe6f2: Subprogram
at=AbstractOrigin val=0x1273d4
at=Lowpc val=0x618bc0
at=Highpc val=0x618c27
at=FrameBase val=0x9c
--- FAIL: TestAbstractOriginSanity (10.00s)
	dwarf_test.go:770: unresolved abstract origin ref in DIE at offset 135569
0xe6f2: Subprogram
at=AbstractOrigin val=0x1273d4
at=Lowpc val=0x618bc0
at=Highpc val=0x618c27
at=FrameBase val=0x9c
--- FAIL: TestAbstractOriginSanityWithLocationLists (11.76s)
	dwarf_test.go:770: unresolved abstract origin ref in DIE at offset 135569
FAIL
FAIL	cmd/link/internal/ld	27.909s

https://storage.googleapis.com/go-build-log/99c11019/linux-amd64_3ca200f0.log

The test has been failing on the BoringCrypto branch since it was introduced in fdecaa8 by @thanm, supposedly due to the BoringSSL syso object.

@FiloSottile FiloSottile added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. labels May 18, 2018
@FiloSottile FiloSottile added this to the Unreleased milestone May 18, 2018
@FiloSottile FiloSottile self-assigned this May 18, 2018
@thanm
Copy link
Contributor

thanm commented May 18, 2018

Let me know if/how I can help.

@FiloSottile FiloSottile removed their assignment May 21, 2018
@FiloSottile FiloSottile changed the title dev.boringcrypto: failing cmd/link/internal/ld test cmd/link/internal/ld: abstract origin missing local variable May 21, 2018
@FiloSottile
Copy link
Contributor Author

Traced this to a Go bug unrelated to BoringCrypto thanks to @heschik.

The DIEs refer to this function, crypto/sha1.New from dev.boringcrypto:

func New() hash.Hash {
	if boringEnabled {
		return boringNewSHA1()
	}
	d := new(digest)
	d.Reset()
	return d
}

Here boringEnabled resolves (across packages) to a true constant, so the rest of the function body, including the local variable, gets eliminated as dead code.

It results in this DW_ABRV_FUNCTION_ABSTRACT DIE, with no local variables:

0x00127364:     TAG_subprogram [3] *
                 AT_name( "crypto/sha1.New" )
                 AT_inline( DW_INL_inlined )
                 AT_external( 0x01 )

0x00127377:         NULL

0x00127378:     TAG_subprogram [3] *
                 AT_name( "crypto/sha1.boringNewSHA1" )
                 AT_inline( DW_INL_inlined )
                 AT_external( 0x01 )

0x00127395:         NULL

It's referenced correctly in a few places as a DW_ABRV_INLINED_SUBROUTINE_RANGES:

0x00128aee:         TAG_inlined_subroutine [6] *
                     AT_abstract_origin( {0x0000000000127364}"crypto/sha1.New" )
                     AT_ranges( 0x0006ee90
                         Base address = 0xffffffffffffffff68f2d0
                         End )
                     AT_call_file( "/usr/local/google/home/valsorda/go/src/crypto/tls/cipher_suites.go" )
                     AT_call_line( 133 )

The problem is when it's referenced as a DW_ABRV_FUNCTION_CONCRETE:

0x00151a6f:     TAG_subprogram [4] *
                 AT_abstract_origin( {0x0000000000127364} )
                 AT_low_pc( 0x000000000066bb80 )
                 AT_high_pc( 0x000000000066bbe7 )
                 AT_frame_base( call-frame-cfa )

0x00151a86:         TAG_variable [12]
                     AT_abstract_origin( {0x0000000000127377} )
                     AT_location(  )

0x00151a8c:         TAG_formal_parameter [14]
                     AT_name( "~r0" )
                     AT_variable_parameter( 0x01 )
                     AT_decl_line( 120 )
                     AT_type( {0x000000000001031f} ( hash.Hash ) )
                     AT_location(  )

0x00151a98:         TAG_inlined_subroutine [6] *
                     AT_abstract_origin( {0x0000000000127378} )
                     AT_ranges( 0x00067700
                         Base address = 0xffffffffffffffff66bb80
                         End )
                     AT_call_file( "/usr/local/google/home/valsorda/go/src/crypto/sha1/sha1.go" )
                     AT_call_line( 122 )

[...]

This time you can see it has a local variable, possibly d, which references the non-existing offset 0x0000000000127377, supposedly where the local variable abstract origin was going to be. The test catches this.

The rest of the DIE reflects just the boringNewSHA1 call, so it is in some way aware of the dead code elimination.

Here's a dwarfinl debug snippet:

assembling DWARF inlined routine info for "".New
  0: II:24 ("".boringNewSHA1) V: ( ) C: ( 1 ) R:
    1: II:25 (crypto/internal/boring.NewSHA1) V: ( h ) C: ( 2 ) R: [29,55)
      2: II:26 (crypto/internal/boring.(*sha1Hash).Reset) V: ( h ) C: ( ) R: [55,64)
V0: h CI:0 II:26 IA:1 param
V1: h CI:0 II:25 IA:1 local
V2: d CI:0 II:-1 IA:1 local
V3: ~r0 CI:1 II:-1 IA:0 param
PutAbstractFunc(go.info."".New$abstract)
putAbstractScope(go.info."".New): vars: 0:d
PutConcreteFunc(go.info."".New)
putscope(go.info."".New,0): vars: 0:0:d 1:1:~r0
PutInlinedFunc(caller=go.info."".boringNewSHA1$abstract,callee=go.info."".boringNewSHA1$abstract,abbrev=6)
PutInlinedFunc(caller=go.info.crypto/internal/boring.NewSHA1$abstract,callee=go.info.crypto/internal/boring.NewSHA1$abstract,abbrev=5)
PutInlinedFunc(caller=go.info.crypto/internal/boring.(*sha1Hash).Reset$abstract,callee=go.info.crypto/internal/boring.(*sha1Hash).Reset$abstract,abbrev=5)

The same happens with crypto/sha512.New.

@thanm, assigning this to you, but let me know if you can't work on it, or if you need help reproducing it.

Also, your dwarf-check tool does not catch this because it does not return an error when Next encounters a NULL at the Offset instead of a DIE.

@FiloSottile FiloSottile removed the Testing An issue that has been verified to require only test changes, not just a test failure. label May 21, 2018
@FiloSottile FiloSottile modified the milestones: Unreleased, Go1.11 May 21, 2018
@thanm
Copy link
Contributor

thanm commented May 22, 2018

Thanks for the analysis. In theory this scenario is supposed to be handled by the existing design (e.g. dead coding of locals) but clearly it's not working quite right. I will take a closer look tomorrow.

@thanm
Copy link
Contributor

thanm commented May 23, 2018

The function of interest in this bug has a variable "d" that gets deleted by dead code elimination; dead variables are (in theory) supposed to be handled cleanly by the DWARF inline generation design, but in this case things are breaking down.

There is a crude dead-code phase (source code in typecheck.go) invoked in gc.Main immediately after type checking; this particular phase will catch things like

  if true {
     ...
  }

however it will not detect things like

  var alwaystrue bool = true
  if x != nil {
     alwaystrue = true
  }
  if alwaystrue {
     return 2
  }
  var iamdead [128]int
  ...

The case above is easily handled by the SSA backend, so 'iamdead' will not survive to the final compiled version of the function. This sort of dead variable doesn't cause any issues for the DWARF code.

The problem here is that in the early deadcode phase, the typecheck.go code eliminates statically unreachable PAUTO nodes from the AST, but it doesn't delete them from the function's "Dcl" list, meaning that the DWARF inlining code still sees them (and emits references to the dead variables).

When export data is generated for a package, however, the export data writer looks at the AST after the deadcode() call invoked by gc.Main(). This means that the version of sha1.New emitted into export data has no locals, which in turn means that some other package that imports "sha1" and invokes New will emit an abstract function for sha1.New that has no locals (since the Dcl list for the inlineable function is reconstructed by the importer).

I think the right way to handle this is to add some code to check for unused PAUTO's (e.g. those that are statically unreachable, like "d" in sha1.New) and not process them in the DWARF code at all. The question is where to do that checking. The typechecker's deadcode phase is a bit too special-purpose for this IMHO; I will look into doing this as part of the inline analysis.

@gopherbot
Copy link

Change https://golang.org/cl/114090 mentions this issue: compiler: fix DWARF inline debug issue with dead local vars

@thanm
Copy link
Contributor

thanm commented May 23, 2018

Thank you also for reporting the problem with dwarf-check. That issue is fixed here.

@golang golang locked and limited conversation to collaborators May 25, 2019
@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 NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants