Clang 18 #202
Clang 18 #202
Conversation
Merge branch '149-migrate-clang-version' of https://github.com/Lind-Project/lind-wasm into 149-migrate-clang-version
|
@yzhang71 there are several merge conflicts here |
| $$CC $$CFLAGS $$WARNINGS $$EXTRA_FLAGS \ | ||
| $$INCLUDE_PATHS $$SYS_INCLUDE $$DEFINES $$EXTRA_DEFINES \ | ||
| -o $$GLIBC_BASE/build/lind_syscall.o \ |
There was a problem hiding this comment.
The merge from main might have removed the lind_syscall compile here
There was a problem hiding this comment.
Yes, but I'm not sure if it's necessary, since lind_syscall.o is pre-shipped in the lind_syscall folder, right?
There was a problem hiding this comment.
This seems like a problem. I believe we changed it to compile every time and not pre-ship since that was causing constant issues.
|
Commit 6e18b65: Build Success |
|
Test Report |
I'm wondering if this is related to Ansal's above lind_syscall.o comment |
Just looked into the reason of the test report failure and I believe it is related to @m-hemmings could you take a look and see if you can fix the issue, so that test could run normally for all PRs? |
|
Commit c18e83d: Build Success |
|
The gen_sysroot.sh was not working because it failed to find the lind_syscall.o while bundling. It can be fixed in two ways. I made the change where we compile lind_syscall during bazel build. If that is to be changed to prepackage lind_syscall.o(like crt1.o), there should be a corresponding change in gen_sysroot.sh mentioning the path. |
Thank you @ansalma6! Can you (or @m-hemmings make sure this stuff is documented somewhere in the build docs) @qianxichen233 does this clarify your concerns? |
|
Test Report |
|
The test report looks much better now. @yzhang71 , could you also take a look at the test report here and compare the test results with those from the main branch? Just want to make sure no additional tests are failing in this PR. |
No problem. I will work on that |
|
The test cases for Clang-16 and Clang-18 have been compared and verified to be the same. |
|
Thanks @yzhang71. I think this is ready for the merge process as you discussed in the meeting Wednesday. @m-hemmings Can you give this another once over, and then I'll have another look. @qianxichen233 can you make sure your requested change is resolved? |
| USER root | ||
| RUN wget http://security.ubuntu.com/ubuntu/pool/universe/n/ncurses/libtinfo5_6.3-2ubuntu0.1_amd64.deb && \ | ||
| apt install ./libtinfo5_6.3-2ubuntu0.1_amd64.deb | ||
| USER lind |
There was a problem hiding this comment.
How is this related to the clang upgrade?
There was a problem hiding this comment.
Would it make sense to move the install before line 32, where the other apt installs are? Then you don't have to change user back and forth.
There was a problem hiding this comment.
How is this related to the clang upgrade?
Yes, we needs this library for using clang-18, otherwise it will give error during runtime.
There was a problem hiding this comment.
Would it make sense to move the install before line 32, where the other apt installs are? Then you don't have to change user back and forth.
Good point! This part is handled by Matt, @m-hemmings Hi Matt, do you think this suggestion can be applied here?
| -c ../sysdeps/unix/sysv/linux/x86/elision-unlock.c \ | ||
| -MD -MP -MF $$GLIBC_BASE/build/nptl/elision-unlock.o.dt \ | ||
| -MT $$GLIBC_BASE/build/nptl/elision-unlock.o | ||
|
|
There was a problem hiding this comment.
Are these related to the clang upgrade?
There was a problem hiding this comment.
I think so, the elision lock and unlock were causing errors, so we need to handle them differently.
Makefile
Outdated
There was a problem hiding this comment.
This file was removed in #214. Is the re-add intentional?
There was a problem hiding this comment.
@m-hemmings Hi Matt, I'm not sure about this.
There was a problem hiding this comment.
Just to confirm, it looks like Makefile was deleted in PR #214 on main, but that commit hasn’t been incorporated into this branch via merge or rebase yet. Is that correct?
scripts/wasmtestreport.py
Outdated
There was a problem hiding this comment.
Are the changes in this file related to the clang upgrade?
There was a problem hiding this comment.
Not really. These changes are intended to make the test report more manageable. Previously, all test cases were displayed in an unordered manner, which made comparison difficult.
| -name "list.o" -o \ | ||
| -name "ldconfig.o" -o \ | ||
| -name "sln.o" \ | ||
| \)) |
There was a problem hiding this comment.
Is the change related to the clang upgrade?
There was a problem hiding this comment.
I think so. These changes were made to resolve function duplication, which occurred when we upgraded to Clang 18.
|
|
||
| # Compiler options | ||
| COMMON_FLAGS := --target=wasm32-unkown-wasi -v -Wno-int-conversion -std=gnu11 -fgnu89-inline \ | ||
| COMMON_FLAGS := --target=wasm32-unknown-wasi -v -Wno-int-conversion -std=gnu11 -fgnu89-inline \ |
There was a problem hiding this comment.
Is this an unrelated bugfix? Was this a problem before?
There was a problem hiding this comment.
I think this was a typo : (
There was a problem hiding this comment.
Sure, but was this an issue? Did the typofix fix the build? Or is this Makefile not used for anything anyway?
There was a problem hiding this comment.
Yeah, this fix the typo. But I'm not sure if it previously causing any problem.
There was a problem hiding this comment.
Some discussion on PR #240's comments
| CFLAGS-elision-lock.c += | ||
| CFLAGS-elision-unlock.c += | ||
| CFLAGS-elision-timed.c += | ||
| CFLAGS-elision-trylock.c += |
There was a problem hiding this comment.
What's the purpose of these empty appends?
There was a problem hiding this comment.
Yes, we need to remove -mrtm because clang-18 no longer support that.
|
Regarding my comments about unrelated changes: Let me know, if you need any help with resolving git history conflicts/issues. |
|
After my huge sweep through the build scripts, it's probably best to start over with this PR (sorry!). The good news is, this should become substantially easier. In terms of replacing clang version strings, you should only need to do this once here: lind-wasm/scripts/Dockerfile.e2e Lines 47 to 48 in 642dae0 ... plus any source code changes needed for the new clang version. |
@yzhang71 I think I can take this from here. |
Description
Fixes # (issue)
Type of change
How Has This Been Tested?
lind_project/tests/test_cases/test_a.clind_project/tests/test_cases/test_b.cChecklist: