-
Notifications
You must be signed in to change notification settings - Fork 353
[libclang][reproducers] Add compilation caching support to reproducers for explicitly-built modules. #11960
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
base: next
Are you sure you want to change the base?
Conversation
…s for explicitly-built modules. Add API to provide CAS options for reproducer generation and use these options to generate compilation commands. Instead of copying relevant files and providing them through VFS, create a new CAS object storage and copy relevant CAS trees there. rdar://158780270
|
An example of API usage is swiftlang/swift-build#964 |
|
|
||
| //--- cas-script-expectations.txt | ||
| CHECK: -fcas-path "reproducer.cache/cas" | ||
| CHECK: "-fcas-include-tree" "llvmcas:// |
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.
I don't know if it is worth it (or common) to check the presence of an include tree in a generated CAS.
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.
For swift, I just find a way to execute the reproducer.
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.
Thanks for the suggestion, it seems to be a more reliable way to check the logic.
| << toString(std::move(Err)); | ||
| } | ||
| } else { | ||
| SmallString<128> VFSCachePath = FileCachePath; |
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.
For the sake of simplifying the review I can outdent this block to show that it matches the previously existing code.
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.
You can just return from the if block to avoid else
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.
The return is non-trivial and I don't want to duplicate it. For now outdented the old code and noticed a mistake.
| if (auto Err = transplantCASIncludeTree(TU.IncludeTreeID)) | ||
| return ReportFailure() | ||
| << "failed to transplant a translation unit include tree due to " | ||
| << toString(std::move(Err)); | ||
| for (const ModuleDeps &ModuleDep : TU.ModuleGraph) { | ||
| if (auto Err = transplantCASIncludeTree(ModuleDep.IncludeTreeID)) | ||
| return ReportFailure() | ||
| << "failed to transplant a module '" + ModuleDep.ID.ModuleName + | ||
| "' include tree due to " | ||
| << toString(std::move(Err)); | ||
| } |
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.
I've made this code not as involved as in Swift reproducers on purpose as I don't have corresponding use cases. @cachemeifyoucan do you have any use cases in mind that I should address?
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.
What use case are you talking about here? clang inputs are entirely captured in include tree but swift doesn't have such a representation so it has to be more involved.
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.
Thanks for the explanation. I didn't know about this difference between swift and clang.
I don't have a specific use case in mind. I was concerned my test case wasn't representative enough and worked by accident. But seems like my approach is sufficient (both in testing and in code).
| //--- cas-script-expectations.txt | ||
| CHECK: -fcas-path "reproducer.cache/cas" | ||
| CHECK: "-fcas-include-tree" "llvmcas:// | ||
| CHECKL "-fmodule-file-cache-key" "reproducer.cache/explicitly-built-modules/Test-{{.*}}.pcm" "llvmcas:// |
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.
CHECKL is a typo?
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.
Yes, thank you so much for noticing it.
|
|
||
| //--- cas-script-expectations.txt | ||
| CHECK: -fcas-path "reproducer.cache/cas" | ||
| CHECK: "-fcas-include-tree" "llvmcas:// |
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.
For swift, I just find a way to execute the reproducer.
| if (auto Err = transplantCASIncludeTree(TU.IncludeTreeID)) | ||
| return ReportFailure() | ||
| << "failed to transplant a translation unit include tree due to " | ||
| << toString(std::move(Err)); | ||
| for (const ModuleDeps &ModuleDep : TU.ModuleGraph) { | ||
| if (auto Err = transplantCASIncludeTree(ModuleDep.IncludeTreeID)) | ||
| return ReportFailure() | ||
| << "failed to transplant a module '" + ModuleDep.ID.ModuleName + | ||
| "' include tree due to " | ||
| << toString(std::move(Err)); | ||
| } |
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.
What use case are you talking about here? clang inputs are entirely captured in include tree but swift doesn't have such a representation so it has to be more involved.
| << toString(std::move(Err)); | ||
| } | ||
| } else { | ||
| SmallString<128> VFSCachePath = FileCachePath; |
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.
You can just return from the if block to avoid else
Add API to provide CAS options for reproducer generation and use these options to generate compilation commands. Instead of copying relevant files and providing them through VFS, create a new CAS object storage and copy relevant CAS trees there.
rdar://158780270