Navigation Menu

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

cmd/compile: possible oversight with internal/gc.disableExport #31049

Closed
odeke-em opened this issue Mar 26, 2019 · 4 comments
Closed

cmd/compile: possible oversight with internal/gc.disableExport #31049

odeke-em opened this issue Mar 26, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@odeke-em
Copy link
Member

I was just studying closure.go and noticed that we invoke disableExport(sym) for closures created.
The signature for disableExport says

go/src/cmd/compile/internal/gc/dcl.go

Lines 1005 to 1006 in 0ec302c

// disableExport prevents sym from being included in package export
// data. To be effectual, it must be called before declare.

However, in the body of disableExport we see that we invoke sym.SetOnExportList(true)
which toggles the symbol to be exported as per

go/src/cmd/compile/internal/types/sym.go

Line 55 in 0ec302c

func (sym *Sym) SetOnExportList(b bool) { sym.flags.set(symOnExportList, b) }

This change is from CL 108216

I believe it should be sym.SetOnExportList(false)
The bug only doesn't show because when creating object files we explicitly check that names are exported as per

go/src/cmd/compile/internal/gc/export.go

Lines 56 to 58 in 0ec302c

if types.IsExported(n.Sym.Name) || initname(n.Sym.Name) {
exportsym(n)
}

otherwise if you add debugs you'll see that we encounter closure symbols such as "".bar.func1

Kindly paging @mdempsky @griesemer
@odeke-em odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 26, 2019
@mdempsky
Copy link
Member

mdempsky commented Mar 26, 2019 •
edited

Thanks @odeke-em , but I believe it's correct as-is. The OnExportList flag is used by exportsym to avoid adding a symbol to exportlist if it's already been added:

go/src/cmd/compile/internal/gc/export.go

Lines 27 to 32 in 0ec302c

// exportsym marks n for export (or reexport).
func exportsym(n *Node) {
if n.Sym.OnExportList() {
return
}
n.Sym.SetOnExportList(true)

The way disableExport works is that it sets OnExportList before exportsym is called, so that exportsym thinks the symbol has already been added to exportlist and avoids adding it again.

@odeke-em
Copy link
Member Author

Thanks for the reply @mdempsky!
Yes, I saw that before but I now understand the purpose of it and context of usage after your reply, thanks!

Perhaps let's rename that helper function to hideFromExportSym instead of disableExporting? disableExporting to me seems like we are requesting that regardless of its case, prevent it from being exported.

Perhaps let's think of a different marking naming, because (*types.Sym).OnExportList for closures within functions (especially those that have names such as "".bar.func1) is a little confusing, just from reading through the source code.

@josharian
Copy link
Contributor

@odeke-em feel free to leave renaming suggestions over at #27167

@odeke-em odeke-em mentioned this issue Mar 27, 2019
Open
@odeke-em
Copy link
Member Author

Cool, thanks @josharian I've made the suggestion at #27167 (comment)

and with that I'll close this issue. Thank you @mdempsky for the response too.

@golang golang locked and limited conversation to collaborators Mar 26, 2020
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.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mdempsky @josharian @odeke-em @gopherbot