[CAS] gmodule support for caching build #11026
Conversation
cachemeifyoucan
requested review from
adrian-prantl,
akyrtzi,
benlangmuir and
jansvoboda11
cachemeifyoucan
force-pushed
the
casify-module
branch
2 times, most recently
from
2bbc405 to
075be6d
Compare
cachemeifyoucan
force-pushed
the
casify-module
branch
from
075be6d to
2a1ffca
Compare
cachemeifyoucan
force-pushed
the
casify-module
branch
from
2a1ffca to
d5d9a15
Compare
There was a problem hiding this comment.
This would potentially fit better into llvm-project/lldb/packages/Python/lldbsuite/
There was a problem hiding this comment.
This is a compiler launcher type of script and requires a just built clang (because it needs clang-scan-deps next to it). I don't what is a good way to hack into lldbsuite or driven from makefile. Let me know if you have any good idea.
cachemeifyoucan
force-pushed
the
casify-module
branch
from
d5d9a15 to
0173e49
Compare
cachemeifyoucan
force-pushed
the
casify-module
branch
2 times, most recently
from
8063aee to
512a886
Compare
cachemeifyoucan
force-pushed
the
casify-module
branch
from
512a886 to
cb036bf
Compare
|
@adrian-prantl @benlangmuir Ping for review |
cachemeifyoucan
force-pushed
the
casify-module
branch
from
cb036bf to
4ae6ba2
Compare
|
@swift-ci please test llvm |
cachemeifyoucan
force-pushed
the
casify-module
branch
from
4ae6ba2 to
5e96cdf
Compare
|
@swift-ci please test llvm |
cachemeifyoucan
force-pushed
the
casify-module
branch
4 times, most recently
from
479c33e to
5c6f640
Compare
|
@swift-ci please test llvm |
cachemeifyoucan
force-pushed
the
casify-module
branch
2 times, most recently
from
11bba2c to
55c97e3
Compare
| /// Check debug info is correct. | ||
| // RUN: %clang %t/tu.o -o %t/a.out | ||
| // RUN: dsymutil -cas %t/cas %t/a.out -o %t/a.dSYM 2>&1 | FileCheck %s --check-prefix=WARN --allow-empty | ||
| // RUN: dsymutil %t/a.out -o %t/a2.dSYM 2>&1 | FileCheck %s --check-prefix=WARN --allow-empty |
There was a problem hiding this comment.
Does this being dsymutil and not %dsymutil mean we might pick up the system one? Since there is also a dsymutil aspect to this, could that be problematic in CI?
There was a problem hiding this comment.
Tools are always in the path.
NOTE: this is not the only test that calls dsymutil in clang tests and dsymutil wasn't even a build dependency for tests (added in this PR).
|
|
||
| def cas : Separate<["-", "--"], "cas">, | ||
| MetaVarName<"<path>">, | ||
| HelpText<"Specify CAS directory to load CAS references from.">, |
There was a problem hiding this comment.
Does this imply that there can only be one CAS per build?
Is that realistic? What if people use static archives and combine objects from different projects?
There was a problem hiding this comment.
Yes, it has to be one CAS per build. It is a requirement for module imports and there is no way you can configure otherwise.
If you have multiple builds and uses some products from other build, the second CAS has to ingest everything from previous builds.
| WithColor::note() << "create CAS using configuration: " << Config->first | ||
| << "'\n"; | ||
| return DB->first; | ||
| } |
There was a problem hiding this comment.
It would be good to find a way to mock this up in a dsymutil-only test that doesn't depend on clang.
There was a problem hiding this comment.
Let me know if you have good ways to do it. It is very hard to do it since it needs to construct both object files (which can be committed) but also module files and CAS contents.
There was a problem hiding this comment.
Usually we check in binary files for dsymutil tests. We only need to resolve a single URL to test it. Is there maybe room for a null CAS plugin where the llvmcas:// URL is a filename directly?
| // START CAS | ||
| FileSpec ModuleListProperties::GetCASOnDiskPath() const { | ||
| const uint32_t idx = ePropertyCASOnDiskPath; | ||
| return GetPropertyAtIndexAs<FileSpec>(idx, {}); |
There was a problem hiding this comment.
How would these settings be communicated to LLDB in the typical use-case?
What if a debug session contains multiple dylibs built against different CAS storage? (I.e., in each project's DerivedData/)
There was a problem hiding this comment.
See ConfigureCASStorage().
This option provides a global overwrite. If no global setting available, it will search for a CAS configuration from the path of the debugging binary upwards to find a CAS configuration file.
cachemeifyoucan
force-pushed
the
casify-module
branch
from
55c97e3 to
a383255
Compare
|
Create PR on stable branch (so test is available) if you like to review over there: #11714 |
| if (!DB) { | ||
| WithColor::error() << "failed to open CAS: " << toString(DB.takeError()) | ||
| << "\n"; | ||
| return EXIT_FAILURE; |
There was a problem hiding this comment.
Can we add tests that check the error handling when:
- no config file is found
- the cas in the config file is corrupted/incompatible/...
- the url cannot be resolved in the cas
| # RUN: %lldb %t/main -s %t/lldb.script 2>&1 | FileCheck %s | ||
| # CHECK: loading module 'Bottom' using CASID | ||
| # CHECK: loading module 'Top' using CASID | ||
| # CHECK: top = (x = 1) |
There was a problem hiding this comment.
Like in the dsymutil patch, it would be important to also test at least the most common error paths (config file doesn't exist, build dir has been removed, CAS is using different file storage format,...)
|
Conceptually this looks good, thanks! |
cachemeifyoucan
force-pushed
the
casify-module
branch
from
a383255 to
6680cb1
Compare
|
@adrian-prantl Can you take a look again? Not all error conditions can be tested or checked due to the limitation of CAS interface. We will not know things like version mismatch. Also all CAS checks are done before checking file system, so we cannot throw messages when CAS lookup failed. I tried best to log/healthcheck most of the CAS loads so we can tell if the CAS read is successful or not with additional logging. |
cachemeifyoucan
force-pushed
the
casify-module
branch
from
6680cb1 to
e7e3bf2
Compare
|
Everything should be addressed, except a standalone dsymutil test case. I need to think how to write that. |
Teach clang to encode CASID as splitDwarfFilename for gmodule when clang caching is enabled. This allows the outputs from compiler do not contain paths to the clang module files, thus allows distributed caching without the need of a unified clang module cache directory path.
Teach dsymutil how to use CAS and how to load clang modules from CAS when building dSYM when gmodule is used.
Teach lldb to load clang modules when gmodule + clang caching is used.
Teach SwiftASTContext to load clang module dependencies from CAS instead of FileSystem.
cachemeifyoucan
force-pushed
the
casify-module
branch
from
e7e3bf2 to
a251643
Compare
3 participants
Prototype for gmodule support by switching
splitDwarfreferences to CASIDs of the module/PCHAlso teach dsymutil to support reading from CAS as a prototype for lldb support.