Skip to content

Ensure that inline_all_trivial_functions() isn't dead-stripped#5280

Closed
steven-johnson wants to merge 5 commits intomasterfrom
srj/inline-all
Closed

Ensure that inline_all_trivial_functions() isn't dead-stripped#5280
steven-johnson wants to merge 5 commits intomasterfrom
srj/inline-all

Conversation

@steven-johnson
Copy link
Contributor

Can break Li2018 in some build environments.

Can break Li2018 in some build environments.
src/Util.cpp Outdated
// and this is only called when load_plugin is called anyway.
// (If there is a better, portable way to ensure this function
// isn't dead-stripped, then let's do that instead.)
(void) Halide::Internal::inline_all_trivial_functions({}, {}, {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough to take the address: internal_assert(&Halide::Internal::inline_all_trivial_functions);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in the configuration I'm using.

@alexreinking
Copy link
Member

alexreinking commented Sep 17, 2020

I'm confused. I thought we agreed in #5228 that statically linking generators wasn't a supported scenario. We had a pretty long discussion about why this isn't portable and @abadams seemed to think it wasn't something anyone was doing anyway?

This is definitely a unix-only setup. On Windows, when you statically link a library, it is gone. There is no dynamic lookup in the library loader there, which is needed for this sort of thing to work with loadable modules. Otherwise you have to use a shared library (on all platforms).

The workaround (that doesn't quite work) that I came up with was to statically link Halide to both the plugin and the generator (and change the interface of the plugins to have a known entry point), but then we get issues with unique naming (although I think that's the only thing).

@abadams
Copy link
Member

abadams commented Sep 17, 2020

I think statically linking to libHalide is a terrible idea, but I guess I should have realized that Google, with its penchant for statically linking the world, is doing it.

Fortunately in that context it's a single system where Steven has full control over the build rules, so it can be made to work.

@alexreinking
Copy link
Member

alexreinking commented Sep 18, 2020

I think statically linking to libHalide is a terrible idea,

👍

Fortunately in that context it's a single system where Steven has full control over the build rules, so it can be made to work.

Then why not link with -Wl,--whole-archive ... -Wl,--no-whole-archive and avoid this PR?

@alexreinking
Copy link
Member

Then why not link with -Wl,--whole-archive ... -Wl,--no-whole-archive and avoid this PR?

From offline discussion: one build post-processes linked binaries (the generator executables) to strip unused symbols. Not sure what a good workaround would be. Maybe some hack via -include?

@steven-johnson
Copy link
Contributor Author

Dropping, I'll figure out another way.

@alexreinking
Copy link
Member

Can this branch be deleted?

@steven-johnson steven-johnson deleted the srj/inline-all branch October 16, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants