Skip to content

Fix missing symbols in LTO builds #16838

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.

Sign up for GitHub

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

Merged
sbc100 merged 1 commit into from
Apr 28, 2022
Merged

Fix missing symbols in LTO builds #16838

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions tools/system_libs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,10 +740,10 @@ class libc(MuslInternalLibrary,
'-Wno-pointer-sign']

def __init__(self, **kwargs):
self.non_lto_files = self.get_non_lto_files()
self.non_lto_files = self.get_libcall_files()
super().__init__(**kwargs)

def get_non_lto_files(self):
def get_libcall_files(self):
# Combining static linking with LTO is tricky under LLVM. The codegen that
# happens during LTO can generate references to new symbols that didn't exist
# in the linker inputs themselves.
Expand All @@ -753,9 +753,7 @@ def get_non_lto_files(self):
# cannot be added to link. Another way of putting it: by the time LTO happens
# the decision about which bitcode symbols to compile has already been made.
# See: https://bugs.llvm.org/show_bug.cgi?id=44353.
# To solve this we put all such libcalls in a separate library that, like
# compiler-rt, is never compiled as LTO/bitcode (see force_object_files in
# CompilerRTLibrary).
# To solve this we force certain parts of libc to never be compiled as LTO/bitcode.
# Note that this also includes things that may be depended on by those
# functions - fmin uses signbit, for example, so signbit must be here (so if
# fmin is added by codegen, it will have all it needs).
Expand Down Expand Up @@ -795,6 +793,11 @@ def get_non_lto_files(self):
iprintf_files = files_in_path(
path='system/lib/libc/musl/src/stdio',
filenames=['__towrite.c', '__overflow.c', 'fwrite.c', 'fputs.c',
'getc.c',
'fputc.c',
'fgets.c',
'putc.c', 'putc_unlocked.c',
'putchar.c', 'putchar_unlocked.c',

@kripken kripken Apr 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do I understand things right that files are put here when LLVM might emit them in LTO ops? That is, there is some LTO opt that could emit a call to getc() that did not exist before.

If so, might it be worth mentioning the source of such calls, like which LLVM pass does so? That might help us prune this list in the future. But if it's hard to do, it doesn't matter I suppose.

@sbc100 sbc100 Apr 28, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes exactly. Don't you think the existing extensive comment above is enough? It gets in to the specifics of llvm's libcall mechanism. Perhaps I can link to specific upstream files and passes though.

@kripken kripken Apr 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The existing comment is probably enough for now. I guess if we need to find the specific LLVM passes that create these calls then we could search in the source.

@dschuff dschuff Apr 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe another option (instead of forcing these files to never be bitcode) is to compile both bitcode and native versions. The bitcode versions get linked into LTO as usual, but then we can add native versions to the native link, which would get pulled in if they weren't in the original link?

@sbc100 sbc100 Apr 28, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That would require some kind of clever linker change though... as of today that linker always finds the first version of a given symbol and ignores all others.

@sbc100 sbc100 Apr 28, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Another way of putting that would require the answer to the question "where is putc defined" to have a different answer before and after LTO.

'printf.c', 'puts.c', '__lockfile.c'])
iprintf_files += files_in_path(
path='system/lib/libc/musl/src/string',
Expand Down Expand Up @@ -1003,6 +1006,12 @@ def get_files(self):

libc_files += glob_in_path('system/lib/libc/compat', '*.c')

# Check for missing file in non_lto_files list. Do this here
# rather than in the constructor so it only happens when the
# library is actually built (not when its instantiated).

@kripken kripken Apr 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# library is actually built (not when its instantiated).
# library is actually built (not when it's instantiated).

for f in self.non_lto_files:
assert os.path.exists(f), f

return libc_files

def customize_build_cmd(self, cmd, filename):
Expand Down