-
Notifications
You must be signed in to change notification settings - Fork 203
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.
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
Avoid rebuilding check-symbols
artifacts
#551
base: main
Are you sure you want to change the base?
Conversation
Makefile
Outdated
# Collect symbol information. | ||
# | ||
"$(NM)" --defined-only \ | ||
$(SYSROOT_LIB)/libc.a $(SYSROOT_LIB)/libwasi-emulated-*.a $(SYSROOT_LIB)/*.o \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunfishcode: one thing that doesn't make a ton of sense is why we collect the defined symbols for libc.a
and libwasi-emulated-*.a
but then when we collect the undefined symbols we switch up which files we're looking at: libc.a
and libc-*.a
. I would have expected these to be the same files in both cases but that results in slightly different undefined-symbols.txt
(e.g., __floatunsitf
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to just check *.a
and *.o
in both places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I can't think of a reason offhand why they're different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, a0cbb41 checks all of the *.a
and *.o
files, which is probably what we want (?). Some things I noticed:
- we don't really check any
*.so
files ever libsetjump.a
is conditioned by$(BUILD_LIBSETJMP)
which is on by default; if someone were to turn this off, we would see check symbols errors (like we do in CI whenBUILD_LIBSETJMP=no
for the "old LLVM" build)
Like other recent PRs, this change uses true file dependencies to avoid rebuilding the files used to check which symbols are defined or not. The true `git diff` test is still run each time, however.
Previously, we generated the defined and undefined symbol lists from different sets of `sysroot/lib` artifacts. This uses all static artifacts that are built, `*.a` and `*.o`, to increase coverage of the symbols wasi-libc is publishing.
d75005b
to
a0cbb41
Compare
|sed 's/.* [[:upper:]] //' \ | ||
|LC_ALL=C sort \ | ||
|uniq \ | ||
> "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I can move all these to separate scripts if we want (here or in a different PR).
Like other recent PRs, this change uses true file dependencies to avoid rebuilding the files used to check which symbols are defined or not. The true
git diff
test is still run each time, however.