ELD support for LTO w/LinkerScripts - #1767
ELD support for LTO w/LinkerScripts#1767Shankar Easwaran (quic-seaswara) wants to merge 2 commits into
Conversation
f51720d to
da7697a
Compare
|
Just pushed the commit to a branch, which I could not rename. Same change as in the |
There was a problem hiding this comment.
Sudharsan Veeravalli (@svs-quic) and Shankar Easwaran (@quic-seaswara), I thought this patch was already in the LTO branch..
Never mind, I see you pulled from LTO branch to 23.x.
| # Check that the input section order within an output section is correct | ||
| # when LTO is used with linker script. | ||
| # RUN: rm -rf %t && split-file %s %t | ||
| # RUN: %python -c "N=100; print('int bar();'); print('int baz();'); [print(f'int foo_{i}() {{ return {i}; }}') for i in range(N)]; print('int main() { return ' + ' + '.join(f'foo_{i}()' for i in range(N)) + ' + baz() + bar(); }')" > %t/1.c |
There was a problem hiding this comment.
We should add the python code in a separate file here.
| Group<grp_ltooptions>; | ||
| def no_lto_linker_scripts : Flag<["--"], "no-lto-linker-scripts">, | ||
| HelpText<"Ignore section mappings specified in " | ||
| "linker scripts during LTO (default)">, |
There was a problem hiding this comment.
What do you think about making --lto-linker-scripts as the default, instead of --no-lto-linker-scripts?
| } | ||
| if (ThisConfig.options().useDefaultPlugins() && | ||
| ThisConfig.options().hasLTOLinkerScripts() && | ||
| !hasAdvancedLTOPlugin(getScript())) { |
There was a problem hiding this comment.
Why do we need the new function hasAdvancedLTOPlugin? We already store the LTOPlugin in ObjectLinker::LTOPlugin, can we add a getter function for that ObjectLinker::getLTOPlugin and use that here?
| plugin::Symbol::Binding Binding, plugin::Section InputSection, | ||
| plugin::Symbol::Kind Kind, plugin::Symbol::Visibility Visibility, | ||
| unsigned Type, uint64_t Size) { | ||
| unsigned Type, uint64_t Size, unsigned SymbolIndex) { |
There was a problem hiding this comment.
Sudharsan Veeravalli (@svs-quic) Why SymbolIndex parameter here was not needed before?
| }; | ||
|
|
||
| auto getBinding = [](const llvm::lto::InputFile::Symbol &Sym) { | ||
| if (!Sym.isGlobal()) |
There was a problem hiding this comment.
If the symbol is weak, then wouldn't !Sym.isGlobal() return true and the function will incorrectly return Symbol::Local?
| @@ -0,0 +1,46 @@ | |||
| # RUN: rm -rf %t && split-file %s %t | |||
There was a problem hiding this comment.
Would it be possible to add a brief comment in tests regarding what the test is testing?
Signed-off-by: Sudharsan Veeravalli <svs@qti.qualcomm.com>
Remove DLL_A_EXPORT from the AdvancedLTO class and keep the implementation internal to the translation unit. The plugin loader only needs the exported C entry points provided by ELD_REGISTER_PLUGIN. This fixes Windows builds where dllexport on the anonymous-namespace class was rejected, and also avoids MSVC exporting implicit copy/assignment members for the unique_ptr-owned module state. Signed-off-by: Sudharsan Veeravalli <svs@qti.qualcomm.com>
6229202 to
2624e3b
Compare
4 participants
No description provided.