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/compile: possible oversight with internal/gc.disableExport #31049

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

Comments

Projects
None yet
3 participants
@odeke-em
Copy link
Member

commented Mar 26, 2019

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

// 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

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

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

@mdempsky

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

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:

// 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

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

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

@odeke-em

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

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.

@odeke-em odeke-em closed this Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.