Should we do selective export for libHalide?

[alexreinking]

(FYI, there is a history here; we used to do selective export, but it proved hard to maintain, for various reasons, and we ended up abandoning it. Opening a new issue for future discussion would be worthwhile.)

Originally posted by @steven-johnson in #4644

---

CMake provides a GenerateExportHeader module that makes it somewhat easier to selectively export library symbols in a cross-compiler-compatible way. That, along with testing discipline to cover all the symbols we want to export, should ease the maintenance burden.

(Please feel free to edit the following two lists)

The pros of doing this include:

• Smaller binary sizes
  • Think of all the inlined template instantiations!
• Explicit API surface, which would make it easier to assign semantic version numbers, rather than dates.
  • If we remove an export declaration, bump the major version; if we add one, bump the minor; if we want to release changes without touching any of these, bump the patch.

Cons:

• Potentially difficult to implement in the Makefile, though it only supports POSIX OSes and GCC-compatible compilers.
• Maintenance was historically a burden. Why?

[abadams]

Halide is designed to be extensible (e.g. Func::add_custom_lowering_pass). People extending Halide may want to reuse all sorts internal tools from outside the compiler. It's also nice to be able to test internal components from an external test, and for external plugins like the autoscheduler to have access to all the internal tools. All this made explicitly exporting symbols a game of whack-a-mole where we were slowly creeping towards exporting everything for one reason or another. It also required littering the source with HALIDE_EXPORT macros. I believe the binary size savings were found to be insignificant (IIRC), largely because we were approaching manually exporting everything.

Regarding versioning - the intent is that namespace Halide is our public API, and changing something there must be done with care because it might break users. But for Halide::Internal we promise nothing (and people using those symbols just have to deal with changes).

So because the binary size difference was close to zero, and we already have an explicit API surface via namespaces, I claim that there are no real pros. Just exporting everything eliminates a whole class of failures at minimal cost.

[alexreinking]

Halide is designed to be extensible (e.g. Func::add_custom_lowering_pass). People extending Halide may want to reuse all sorts internal tools from outside the compiler. It's also nice to be able to test internal components from an external test, and for external plugins like the autoscheduler to have access to all the internal tools.

Between this and:

But for Halide::Internal we promise nothing (and people using those symbols just have to deal with changes).

Why would anyone want to depend on Halide::Internal? We might decide to, for example, stop exporting any symbols in that namespace. If we want to weigh those users' needs in our decisions about Halide::Internal, then we don't actually have an internal/external distinction.

But then everything else (tests, autoschedulers) is first-party and we can adjust our build processes accordingly to either give those parts access to the internal symbols, or promote relevant symbols to the public API.

I believe the binary size savings were found to be insignificant (IIRC), largely because we were approaching manually exporting everything.

Did we try applying the relevant linker flags (eg. -Wl,--exclude-libs,ALL) to LLVM, too? I'm surprised that dropping the over 120k symbols between LLVM and Halide::Internal had no impact on binary size or loading times (which would be observable in the tests)

[abadams]

Not sure what flags we tried, you may want to experiment. For llvm we already dead-strip all symbols not used by Halide itself in the makefile build via linker shenanigans.

We care about external users of both Halide:: and Halide::Internal::, but there are different expectations around each. Users of Halide::Internal:: should expect to have to tweak their code every time they update Halide (in the same way we have to tweak our code every time we update LLVM). Users of Halide:: expect more stability than that (e.g. there are deprecation warnings for a while before we delete things).

[alexreinking]

Not sure what flags we tried, you may want to experiment. For llvm we already dead-strip all symbols not used by Halide itself in the makefile build via linker shenanigans.

It is probably the right move, then, to do the experiment, and see if there are actual gains to be had. Those can be weighed against the ongoing maintenance burden of marking down exports.

[abadams]

The problem is to do the experiment you'd need to mark all the exports, which is more than just everything in Halide::, because of the extensibility stuff I mentioned. I think everything mentioned in Halide.h is the right set of symbols, as those are the things we've decided should have declarations in user code. Unfortunately that's nearly everything. We just exclude LLVM-using stuff and some built-in tables I think.

One thing you could do is roll back the repo to the point where we deleted all the export macros and check the size and loading time of libHalide before and after that commit. That should be a reasonable proxy for what it would look like to reintroduce the macros.

[alexreinking]

I've started running into issues on Windows with CMake 3.19 regarding exported symbols not being found. This might have to do with WINDOWS_EXPORT_ALL_SYMBOLS flakiness; remember: this is a CMake/Kitware-maintained convenience feature, not just another flag to pass to a Microsoft tool.

Still investigating. But if this is the problem, then selective export might be worth our while.

[dsharletg]

A bit of data here: #5548 reduces the number of non-LLVM symbols in libHalide.so by 25-50%, to around 3,000 symbols. However... there are ~30,000 symbols coming from LLVM. It seems like maybe we should try to figure out how to link LLVM and give all of its symbols internal linkage. This might also help with linker issues when people use libHalide.so and LLVM together in another project.

[dsharletg]

I guess if anything in Halide.h uses LLVM types, that might not work well.

[steven-johnson]

I guess if anything in Halide.h uses LLVM types, that might not work well.

That would be an error if it is currently the case. In theory, only Halide source files that include LLVMHeaders.h should reference any LLVM types.

[alexreinking]

I guess if anything in Halide.h uses LLVM types, that might not work well.

I'm pretty sure we require that this doesn't happen. So if it does, somehow, it's a bug.

It seems like maybe we should try to figure out how to link LLVM and give all of its symbols internal linkage. This might also help with linker issues when people use libHalide.so and LLVM together in another project.

There are two ways I know of:

1. Explicitly list what we want to export (my preferred approach). This has the advantage of being somewhat portable in that all major compilers have explicit symbol visibility controls and CMake can abstract over them with a generated header and macro. This approach is technically required on Windows, though CMake has an exclusive feature that scans the object files to generate a Microsoft-linker-specific script (see below) to emulate GNU defaults.
2. Use a linker-dependent script to exclude symbols. GNU ld supports a thing called "version scripts" that we could use in theory. I'm mentioning this out of completeness, but I would strongly advocate for going in a more portable direction. Obviously a GNU ld linker script would not work on macOS or Windows.

[steven-johnson]

I'm claiming this, and (gradually) working on implementing.

[steven-johnson]

As I look at this, there are some preconditions to making this work:

• We take a fast-and-loose approach about implementation public API in our header files, sometimes as explicitly inline functions, sometimes not (and sometimes with constructs like inline HALIDE_NO_USER_CODE_INLINE). IMHO, we should take a hard line here and institute something like:

  • Anything in the intended public API of Halide should have its function body in a .cpp file, not a .h file.
  • Templated classes and functions should be kept as 'thin' as possible in the Halide API; ideally, these should all be mostly-inlined wrappers around non-templated class/functions.

• In theory, the Halide namespace is our public API, but there are a lot of internal things that need exposing for our current tests. I'm distinguishing the two as I go via syntactic means (eg HALIDE_EXPORT_FOR_TEST for things that in theory are only exposed for existing tests), but in practice, anyone could use these. In the long run, we should find ways to reduce/eliminate the need to expose 'extra' stuff for tests (and/or promote some of these to 'real' API status).

• Related: there are classes in the Internal namespace that are exposed either as necessary base classes for our public API (eg GeneratorStub) or as a possible-accidental side-effect (eg Func::get_schedule(), which returns a StageSchedule).

[abadams]

EXPORT_FOR_TEST is a bit of a misnomer, as these things are also necessary to export for anyone writing a compiler extension, which is something we explicitly support. Pipeline::add_custom_lowering_pass is in the public API, and custom lowering passes might profitably use anything from Halide::Internal. We want to support people doing novel compiler extensions, so trying to hide things in Internal is counterproductive.

The real difference between Internal and not is the expectation for stability. We'll change things in Internal whenever we feel like it. We're much more careful about deprecating things outside of that namespace.

Agree very much that we should move things out of headers wherever practical (e.g. the lambda PR).

[steven-johnson]

I think we have (mostly) resolved this via #5659, though it's still an open issue for Windows. Would anyone like to take this on for MSVC so that we can finally close this?

[alexreinking]

I think I need to learn a bit more about Windows development, here. The last time we tried this on MSVC, we ran into issues with the way Windows is more paranoid about exporting functions/classes with C++ standard library types in their signatures/inheritance hierarchies (e,g. our error type that inherits from std::exception. The compiler refuses to do it.

I think we concluded that adding a way to filter CMake's WINDOWS_EXPORT_ALL_SYMBOLS feature would be a low-resistance path, but upstream didn't bite (or reject us, just saw it as low-prioritiy IIRC).

[LebedevRI]

Semi-related: #5659 (comment)