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

proposal: cmd/compile: expose -ptabs/-exporttypes flag to force compiler to emit ptabs/type info #58567

Open
eh-steve opened this issue Feb 16, 2023 · 12 comments
Labels
Milestone

Comments

@eh-steve
Copy link

Currently compiler only emits ptabs for package main with -dynlink, for -buildmode=plugin. Access to type information for package exports may be useful in other contexts, and is a fairly unobtrusive change.

@gopherbot gopherbot added this to the Proposal milestone Feb 16, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/runtime

@ianlancetaylor
Copy link
Contributor

Is this a format that we want to commit to supporting forever?

@eh-steve
Copy link
Author

eh-steve commented Feb 16, 2023

Good question, not sure if it's aimed at me though...? Do all gcflags fall under the compatibility guarantee? This doesn't change any behaviour in the current compiler, and I don't see how it differs from other flags like DebugFlags.PCTab... Unless you meant the ptab binary format?

@ianlancetaylor
Copy link
Contributor

The compiler flags don't fall under the compatibility guarantee, but still once we start supporting something it's hard to stop.

@cherrymui
Copy link
Member

Could you explain more of your use case? I think what (and whether) to emit and the format would depend on how the data will be consumed. Thanks.

@eh-steve
Copy link
Author

eh-steve commented Feb 17, 2023

@ianlancetaylor
Copy link
Contributor

Thanks--can you copy that text to this issue?

@eh-steve
Copy link
Author

eh-steve commented Feb 17, 2023

Sure, I’ve been working on a JIT compiler (https://github.com/eh-steve/goloader/tree/master/jit#usage-and-configuration) which can build, load and unload arbitrary Go code and its dependencies from a running host binary, using the standard go toolchain (but bypassing the final link step - it uses archives directly from the compiler). It differs from std plugin because code can be unloaded/replaced, symbols are deduplicated across the host binary and the JIT code where possible (without needing the host binary to mark all methods reachable, unlike plugin), and it works with or without CGo (no dependency on libdl), and therefore on Windows.

My (unsatisfying, but working) approach so far has been to first parse the JIT package and use the *syntax.FuncDecl and *syntax.VarDecl nodes to find all exports, and generate a function for each export to retrieve its *_type via reflection, and writing this temporary generated file into the package before actually compiling. This requires the package source path to be writable, and to ensure this is possible, I introduced a bunch of unnecessary module copying.

Since this is an optional gcflag, it doesn't change the current compiler behaviour except when set, and doesn't prevent linker deadcode elimination, except for selected packages where the flag is set.

I think the previous proposal wanted to always export ptabs for all exports in the main binary's moduledata to provide a string-to-function lookup (preventing deadcode elimination) which I agree would be unnecessary.

@eh-steve
Copy link
Author

eh-steve commented Mar 20, 2023

As an alternative, I'd also be happy with a flag to tell the compiler to emit type data for global functions by skipping the ir.PFUNC check in cmd/compile/internal/gc.dumpGlobal(), if we wanted to avoid depending on the ptab binary format (i.e. emit GoAuxTypes)

@randall77
Copy link
Contributor

dumpGlobal just affects DWARF. Is that all you need?

@eh-steve
Copy link
Author

eh-steve commented Apr 10, 2023

dumpGlobal just affects DWARF. Is that all you need?

For my particular use case, I think so?

As far as I can tell, dumpGlobal still forces the inclusion of the runtime types into the archive because TypeLinksym() is called, and this ties the type symbol and global symbol together (via GoAuxType) in a way I can reconstruct an interface{} from, though I haven't tested this end to end yet.

This would need to run dumpGlobal on Exports as well as Externs though, and would need to handle the possibility of inclusion of duplicate symbols. Edited, see below

If you think that's a less contentious approach, I'm happy to adjust the proposal?

@eh-steve
Copy link
Author

eh-steve commented Apr 11, 2023

Following up on this, I don't actually need to dumpGlobal at all since I don't use the DwarfGlobal, I think I'd just need something like this inside a conditional at the top of dumpdata():

	for _, export := range  typecheck.Target.Exports {
		s := export.Linksym()

		if strings.HasSuffix(s.Name, "..inittask") && s.OnList() {
			continue
		}

 		t := export.Type()
		if t == nil || (t.IsPtr() && t.Elem() == nil) || t.IsUntyped() {
			continue
		}
		s.Gotype = reflectdata.TypeLinksym(export.Type())
	}

This seems to run ok without any ICEs, but might need some tests updating?

I'd imagine the linker deadcode pass will strip these surplus symbols from the final binary if they're not needed, so this might be fairly harmless, as it only affects compiler output (though I haven't checked)

@eh-steve eh-steve changed the title proposal: cmd/compile: expose -ptabs flag to force compiler to emit ptabs proposal: cmd/compile: expose -ptabs/-exporttypes flag to force compiler to emit ptabs/type info May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants