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

Jump to bottom

debug/plan9obj: add ErrNoSymbols sentinel for case that no symbol section exists #48052

Closed
kortschak opened this issue Aug 30, 2021 · 9 comments
Closed

debug/plan9obj: add ErrNoSymbols sentinel for case that no symbol section exists #48052

kortschak opened this issue Aug 30, 2021 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals (old)
Milestone
Backlog

Comments

@kortschak
Copy link
Contributor

The elf package provide a sentinel error, ErrNoSymbols for the case that Symbols or DynamicSymbols cannot return a symbol list because the section is empty. The same situation is handled in plan9obj by returning a new error for this case. For PE/Mach-O there is no issue since access to the symbols is via direct access to a slice field.

Because of this, it's either necessary to do an error string comparison or re-call f.Section("syms") to check whether is was nil, repeating work that has already been done.

Originally posted to golan-nuts: https://groups.google.com/g/golang-nuts/c/HUpyQwYVKZE/m/uEOw3N4JGwAJ, but filing here to avoid issue being lost.

I am happy to send a PR if this is acceptable.

What version of Go are you using (go version)?

$ go version
go version go1.17 linux/amd64

Does this issue reproduce with the latest release?

Yes
@cherrymui
Copy link
Member

Is error string comparison problematic? I'm not sure if we want to avoid error string comparison and replace errors.New with sentinel errors.

cc @rsc

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 31, 2021
@cherrymui cherrymui added this to the Unplanned milestone Aug 31, 2021
@kortschak
Copy link
Contributor Author

kortschak commented Aug 31, 2021 •
edited

The rationale for using a sentinel is that the sentinel approach is already used in debug/elf. Not using string comparison comes from the fact that the error return is not documented and I have seen numerous times in code reviews that comparison of errors by string value is frowned upon. Given these, and being presented with the option of string comparison or rechecking the system state directly by examining f.Section("syms"), I would prefer the latter.

However, adding a sentinel error clearly documents the failure mode, reflects the approach used in the related API in elf, and is more efficient and ergonomic than string comparison.

@ianlancetaylor ianlancetaylor changed the title debug/plan9obj: no ErrNoSymbols sentinel for case that no symbol section exists proposal: debug/plan9obj: add ErrNoSymbols sentinel for case that no symbol section exists Sep 10, 2021
@ianlancetaylor
Copy link
Contributor

Turning this into a proposal. It seems fine to me.

@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Sep 10, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 10, 2021
@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

Making this work just like elf's ErrNoSymbols seems fine.

@gopherbot
Copy link

Change https://golang.org/cl/350229 mentions this issue: debug/plan9obj: export ErrNoSymbols

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 20, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Oct 27, 2021
@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: debug/plan9obj: add ErrNoSymbols sentinel for case that no symbol section exists debug/plan9obj: add ErrNoSymbols sentinel for case that no symbol section exists Oct 27, 2021
@rsc rsc modified the milestones: Proposal, Backlog Oct 27, 2021
@gopherbot
Copy link

Change https://golang.org/cl/373774 mentions this issue: doc/go1.18: mention debug/plan9obj.ErrNoSymbols

gopherbot pushed a commit that referenced this issue Dec 23, 2021
@ianlancetaylor
doc/go1.18: mention debug/plan9obj.ErrNoSymbols
0e4b878 
For #47694
For #48052

Change-Id: I136be9b498033309d876099aae16bad330555416
Reviewed-on: https://go-review.googlesource.com/c/go/+/373774
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@ianlancetaylor ianlancetaylor mentioned this issue Feb 24, 2022
Closed
@golang golang locked and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
No open projects
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @kortschak @ianlancetaylor @gopherbot @cherrymui