-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[modules] -print-file-name=libc++.modules.json does not work in mac m1 #109895
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
Comments
@llvm/issue-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9)
I received a private report that `-print-file-name=libc++.modules.json` doesn't work well in mac. Maybe this is related to the installation path of libc++ in MacOS.
Verify needed. |
Also it looks like
but in mac m1, the libc++ modules json file is installed to
then |
@llvm/issue-subscribers-clang-driver Author: Chuanqi Xu (ChuanqiXu9)
I received a private report that `-print-file-name=libc++.modules.json` doesn't work well in mac m1. Maybe this is related to the installation path of libc++ in MacOS.
Verify needed. |
I guess we need help from Apple's people @Bigcheese @jansvoboda11 @ldionne |
Not sure if this helps but: % clang++ -print-file-name=libc++.modules.json clang++ --version |
This seems similar to #98325. @Xazax-hun Any appetite for this one? |
Some additional information about the codes, hope this helps: The
and in |
The following patch seems to work for me: index efe398dd531d..78d06a68bf0d 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -192,8 +192,12 @@ std::string Driver::GetResourcesPath(StringRef BinaryPath) {
P = llvm::sys::path::parent_path(Dir);
// This search path is also created in the COFF driver of lld, so any
// changes here also needs to happen in lld/COFF/Driver.cpp
- llvm::sys::path::append(P, CLANG_INSTALL_LIBDIR_BASENAME, "clang",
- CLANG_VERSION_MAJOR_STRING);
+ llvm::sys::path::append(P, CLANG_INSTALL_LIBDIR_BASENAME);
+ // Darwin and AIX does not use per-target runtime directory.
+ if (!llvm::Triple(llvm::sys::getProcessTriple()).isOSDarwin()) {
+ llvm::sys::path::append(P, "clang",
+ CLANG_VERSION_MAJOR_STRING);
+ }
} |
A minor nit: the comment mentions AIX but the code doesn't. |
Yes, I think the code should be updated to do the same for AIX, this comment was copied from another location. Wanted to make sure this is the right approach before cleaning it up. If this looks good to @ChuanqiXu9 and others, I can make a PR with cleaned up code. |
It looks good to me. But I think we need double check from apple forks or Driver owners @jansvoboda11 @Bigcheese @MaskRay |
Any way to move this along? Should I just create a PR? |
I think that would be the quickest way to ensure this gets addressed, since you already have the patch. |
Yes, +1 |
Oh dear... I created a fork and went to compile and now I am getting a new error. This is from the bootstrapped compiler not being able to find stdbool.h. I think it is some update of xcode that has caused this. Anyone seen this? |
I didn't see it. But I think we didn't force contributors to bootstrap their build as far as I know. So maybe we can workaround that. |
I created an issue: #112151 (comment) |
@ChuanqiXu9 do you know anything about how the default include paths get constructed for the clang that is in the build tree I think I found the issue. See my last comment in the issue I created. |
I think it is |
OK, the patch is what is breaking the build. So, my patch does not work. I think perhaps the location of libc++.modules.json should be changed on the mac to match where it is on linux, so that this works. |
@petrhosek can you look at this? I think it is related to the install location of libc++.modules.json. Using git blame I can see you put this code in: |
OK, I have another possible patch. This one does not break the build.
|
Although it might be a little bit add hoc, it is much better than the existing status. I think you can send a PR after you add a test. For the test, you can take a look at: clang/test/Driver/modules-print-library-module-manifest-path.cpp |
Yeah, it did feel a bit ugly. But I think this is a new place for files like this to be found. There is actually some code right above this that looks in "..", just not "../..". Clearly the test is bad, I will see if I can get a test in there that fails the way it is now. |
@ChuanqiXu9 I had some time to look at the test a bit more and I think I see why it is passing when it should not. It is hard coded to only work for linux.
It basically creates a libc++.modules.json file in a temp dir. Then finds it. Of course this matches what we are seeing that this only works for x86_64 linux as that is all that is being tested. I guess I could work on a version that finds the one in the real build tree and not a mock one, but is that done in clang? @ChuanqiXu9 looks like you wrote this test, so perhaps you can help me figure out how to fix it? Maybe something like this:
But not sure how to come up with the regex that would work for all builds to put in libcxx.cpp.actual. |
No, we shouldn't do this in clang's lit test. We shouldn't lookup the file in the real build tree. Otherwise it will be problematic for other devs if they don't have the same environment.
So we can't do this. We have to add other arguments to make clang to search the files in the new root we specified. I think we can improve this by making the mocked tree more real. |
I am not so sure about that. What if the file is not even installed or created. The way this test worked you would never find that out. Isn't there a way to just run the compiler and get the full path out of it for the actual file and test if that file exists? |
Seems like we could just run the command and make sure the file that it produces actually exists. That should not cause any trouble for any dev or build. Since -print-file-name=libc++.modules.json gives a full path to the file, we should just be able to check and see if it is there or not. If we just try and make the mocked tree more real we will likely miss cases. What if someone changes where it is installed, then the test will pass but the code will be broken. The following works. It fails before I make my change, and passes with my patch.
Issues I can see with this are that test -f would not work on Windows. However, there are a few test -f calls in that test directory so maybe it is ok.
|
No, we shouldn't do this in clang. It is best to make the lit test in clang self contained that it may not affect by the environment of different devs.
I don't think we want it in clang lit test. In practice, if any did so, we will receive issue reports. |
My patch and the test do work. If you have another suggestion I would be open to it. However, I think my test should work for all developers. |
What if the dev doesn't install the std module? Then what's the return value of |
Same with #120215 (comment) to add help wanted label. |
Not surprising to me that it is failing on other systems. Can we set this up so this test is only run when the std module is installed? I don't think it would be possible to correctly simulate this so that it verifies that it works on all platforms unless we actually test the real compiler itself. |
I don't think we did this in lit test.
Test is super important but it has layers. For the in-tree test of clang, we will only do the mocking test. It is not enough for sure. Then we depends on the test from users. Including cmake. e.g., when something go wrong, cmake or any user can report that. |
I have this problem with Intel Macs as well. This is because But the workaround is here. for Intel Mac for Apple silicon Mac – Change line 13 |
Maybe we should just apply my patch since this isn't tested anyway. It should fix a bunch of issues people are seeing. I seriously don't see how this can be tested correctly without testing the actual install. If anything is changed in the location of where this file goes in the actually install, the tests will keep on passing. We know my patch will fix the issues on Mac. |
We have our policy. It should be tested with a mocked installed layout. |
OK, I think I have it working with a mock test. The test fails without the change to Driver.cpp and passes with it. @ChuanqiXu9 Does this look good? If so, I can do a PR.
|
Thanks. It looks good to me. |
@ChuanqiXu9 here is the PR #122370 |
I received a private report that
-print-file-name=libc++.modules.json
doesn't work well in mac m1. Maybe this is related to the installation path of libc++ in MacOS.Verify needed.
The text was updated successfully, but these errors were encountered: