Skip to content

Fix missing symbols in LTO builds - #16838

Merged
sbc100 merged 1 commit into
mainfrom
fputc_lto
Apr 28, 2022
Merged

Fix missing symbols in LTO builds#16838
sbc100 merged 1 commit into
mainfrom
fputc_lto

Conversation

@sbc100

@sbc100 sbc100 commented Apr 28, 2022

Copy link
Copy Markdown
Collaborator

Add more symbols from LLVM's list of libcalls to the list of non-LTO
files in libc.

Sadly it quite hard to write robust tests for this kind of thing as it
depends on LLVM internals.

Fixes #16836

Add more symbols from LLVM's list of libcalls to the list of non-LTO
files in libc.

Sadly it quite hard to write robust tests for this kind of thing as it
depends on LLVM internals.

Fixes #16836
@sbc100
sbc100 enabled auto-merge (squash) April 28, 2022 15:45
@sbc100
sbc100 requested review from dschuff and kripken April 28, 2022 15:45

@kripken kripken left a comment

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.

Side note, I guess this is another benefit of Binaryen's LTO-like opts since some of these functions might be small and worth inlining...

Comment thread tools/system_libs.py
'fputc.c',
'fgets.c',
'putc.c', 'putc_unlocked.c',
'putchar.c', 'putchar_unlocked.c',

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.

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.

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.

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?

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.

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.

Comment thread tools/system_libs.py

# 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).

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).

@sbc100
sbc100 merged commit fb83ec8 into main Apr 28, 2022
@sbc100
sbc100 deleted the fputc_lto branch April 28, 2022 18:30
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.

Link failing with attempt to add bitcode file after LTO

3 participants