Dependency Scanning Library #34786

artemcm · 2020-11-17T20:06:49Z

This PR introduces a dependency scanning tool/library that is meant to be used by libSwiftDriver, replacing its current calls to swift-frontend -scan-dependencies.

This library is designed to be used for dependency scanning across multiple, different modules, with a common ModuleDependencyCache, in order to re-use computed information about already-scanned modules.

This PR:

Adds a C++ interface for a tool that answers dependency-scanning queries (DependencyScanningTool).
Adds a unittest harness to invoke the DependencyScanningTool library.
Factors ModuleDependenciesCache out of a CompilerInstance into callers of scanDependencies. It is now the responsibility of the scanDependencies code to instantiate (and share) the cache. e.g. FrontendTool instantiates a new cache per -scan-dependencies invocation, and the DependencyScanningTool keeps one shared cache across its lifetime. (Effectively un-doing [Dependency Scanner] Share ModuleDependenciesCache as a part of the CompilerInstance #34646)
Deprecates and removes -scan-clang-dependencies. Since it was introduced, its use-case has been entirely subsumed by batch scanning.
Adds a C API layer for dependency scanning tool, scanning actions, and scan results. (include/swift-c/DependencyScan)
Refactors dependency scanner to return an in-memory format as its result output. Specifically, code in ScanDependencies.cpp is split into two functional groups:
- Scan execution code that performs the mechanics of the scan and produces an in-memory result
- Dependency scanner entry-points used when the scanning action is invoked as a swift-frontend execution mode
- Adds the aforementioned in-memory dependency scanning result in DependencyScan.h, modelled after the InterModuleDependencyGraph representation in swift-driver

Resolves rdar://71209248
Resolves rdar://71209311

lib/SwiftScan/CMakeLists.txt

lib/SwiftScan/ScanDependencies.cpp

artemcm · 2020-11-20T22:45:51Z

@swift-ci please smoke test

artemcm · 2020-11-20T22:47:23Z

@nkcsgexi, curious to hear your thoughts on the refactor in ScanDependencies.h and ScanDependencies.cpp.

nkcsgexi · 2020-11-20T23:21:30Z

They look good to me for now. Can we first define the C header defining those APIs libSwiftDriver need for scanning and refactor the C++ implementation accordingly?

artemcm · 2020-11-20T23:24:10Z

They look good to me for now. Can we first define the C header defining those APIs libSwiftDriver need for scanning and refactor the C++ implementation accordingly?

I like this idea, yeah, I'll do that.

artemcm · 2020-12-02T19:58:38Z

@akyrtzi @benlangmuir,
I would be grateful to get your comments on the C API, among other things, mostly contained in:
include/swift-c/DependencyScan

artemcm · 2020-12-02T19:59:01Z

Likewise, @nkcsgexi.

include/swift-c/DependencyScan/DependencyScan.h

include/swift/DependencyScan/DependencyScanImpl.h

include/swift-c/DependencyScan/DependencyScan.h

akyrtzi · 2020-12-05T05:30:03Z

Some additional things for your consideration:

Adding an exports list file would be general goodness. It is what will give you a linker error if an exported function is not implemented (e.g. when some type or function changes and the function implementation doesn't match the declaration) and can help to strip the library of unused code and make it smaller.
ds as prefix is too short. Consider that you are essentially adding symbols to the global namespace of a client's process (any client, not necessarily something we control), and the prefix is the "defense" against unintentional conflicts with other libraries. I'd suggest to use a more discriminating prefix, like swiftds_ or swiftscan_
Consider using _Nonnull and _Nullable to make the APIs more ergonomic for Swift
I'd recommend to base your testing on top of the C API. It will be a bit less convenient than testing using the C++ APIs but you will have piece of mind that you are really testing what the client is actually using.

benlangmuir · 2020-12-07T21:32:13Z

include/swift-c/DependencyScan/DSString.h

 */
 typedef struct {
  const void *data;
  unsigned private_flags;
-} ds_string_t;
+} swiftscan_string_t;


@akyrtzi do you know why CXString has this representation? I was surprised it's not just a pointer.

The only use of the flags seems to be to distinguishing who manages this pointer, whether it was malloc'd locally or points to unmanaged memory.

I personally dislike CXString, I'd suggest to either use something like indexstore_string_ref_t and be clear what is the lifetime of the returned string (e.g. "same lifetime as the object the string came from") or malloc and return a char * and require the caller to free() it.

be clear what is the lifetime of the returned string (e.g. "same lifetime as the object the string came from")

Note that on the Swift code you are mainly just going to copy it into a Swift string as soon as you call it, so the lifetime aspect it won't make things complicated (it will even simplify things since you won't have to call dispose on it).

I changed the string implementation to one that resembles index store_string_ref_t and its lifetime is now tied to the object the string comes from. Clients need not worry about freeing string memory, it will be freed automatically when the owning object is disposed of.

benlangmuir · 2020-12-07T21:37:42Z

include/swift-c/DependencyScan/DependencyScan.h

+swiftscan_clang_detail_get_module_map_path(swiftscan_module_details_t details);
+
+SWIFTSCAN_PUBLIC swiftscan_string_t
+swiftscan_clang_detail_get_context_hash(swiftscan_module_details_t details);


These accessors that return swiftscan_string_t or swiftscan_string_set_t * are not clear about ownership. I see you have swiftscan_string_dispose and swiftscan_string_set_dispose methods, but the implementation looks like they are returned unowned. If not every API is consistent about whether the caller needs to dispose of the result, I recommend putting it in the doc comment for the accessors.

Resolved by tying the lifetime of the string to the object it is associated with. swiftscan_string_dispose and swiftscan_string_set_dispose are no longer part of the public API.

In general, I propose we adopt Foundation-style ownership signals here

In general, I propose we adopt Foundation-style ownership signals here

@CodaFi, thank you taking a look at this PR and proposing this convention.
The API now consistently follows this naming scheme, providing a create and dispose functions for all objects that the library's clients may take ownership of.

artemcm · 2020-12-09T00:06:54Z

Adding an exports list file would be general goodness. It is what will give you a linker error if an exported function is not implemented (e.g. when some type or function changes and the function implementation doesn't match the declaration) and can help to strip the library of unused code and make it smaller.

Already caught an error by doing this, thank you @akyrtzi!

CodaFi · 2020-12-09T01:05:13Z

tools/libSwiftScan/libSwiftScan.exports

@@ -0,0 +1,51 @@
+swiftscan_dependency_result_get_main_module_name


You may have some entertaining conflicts with this.

CodaFi · 2020-12-09T01:07:28Z

include/swift-c/DependencyScan/DependencyScan.h

+//
+//===----------------------------------------------------------------------===//
+
+#ifndef SWIFT_C_DEPENDENCY_SCAN_H


"llvm/Support/CBindingWrapping.h" provides some utilities that might help things here.

… if it is `empty`

…ay struct type fields to be consistent.

…used for batched versioned-PCM scans.

…side string construction (Using string length provided by the type)

…ll of our build compilers.

…hen collecting cross import overlays

…h up-to-date search paths on successive scanner invocations To ensure they do not get stuck with stale search paths when scanning successive targets.

…creation to populate LLVM's target data store.

artemcm · 2021-01-07T17:09:09Z

@swift-ci please test

swift-ci · 2021-01-07T18:44:31Z

Build failed
Swift Test Linux Platform
Git Sha - cc400bf

artemcm · 2021-01-07T21:07:28Z

@swift-ci please test Linux platform

artemcm · 2021-01-07T21:40:18Z

@swift-ci please test

artemcm · 2021-01-07T22:40:59Z

@swift-ci please test Windows platform

artemcm · 2021-01-07T23:45:56Z

@swift-ci please test Windows platform

…opefully work better on windows.

artemcm · 2021-01-08T00:53:41Z

@swift-ci please test Windows platform

swift-ci · 2021-01-08T01:05:33Z

Build failed
Swift Test OS X Platform
Git Sha - f5429870d84d5be6dd03a076bbf07aa8ae02208b

artemcm · 2021-01-08T01:51:50Z

@swift-ci please test Linux platform

artemcm · 2021-01-08T01:51:59Z

@swift-ci please test macOS platform

swift-ci · 2021-01-08T06:47:27Z

Build failed
Swift Test OS X Platform
Git Sha - 4eae21c

artemcm · 2021-01-08T15:45:28Z

@swift-ci please test macOS platform

swift-ci · 2021-01-08T19:05:08Z

Build failed
Swift Test OS X Platform
Git Sha - 4eae21c

artemcm · 2021-01-08T19:50:16Z

@swift-ci please test macOS platform

nkcsgexi · 2021-01-09T00:04:31Z

🎉

artemcm requested a review from nkcsgexi November 17, 2020 20:06

artemcm commented Nov 17, 2020

View reviewed changes

lib/SwiftScan/CMakeLists.txt Outdated Show resolved Hide resolved

nkcsgexi reviewed Nov 17, 2020

View reviewed changes

lib/SwiftScan/ScanDependencies.cpp Outdated Show resolved Hide resolved

artemcm force-pushed the DependencyScanLibrary branch 2 times, most recently from f1dd0aa to 9f5fab0 Compare November 20, 2020 22:44

artemcm force-pushed the DependencyScanLibrary branch 4 times, most recently from 6294902 to fb9ba46 Compare December 2, 2020 18:50

artemcm requested review from akyrtzi and benlangmuir December 2, 2020 19:57

nkcsgexi reviewed Dec 2, 2020

View reviewed changes

include/swift-c/DependencyScan/DependencyScan.h Outdated Show resolved Hide resolved

artemcm mentioned this pull request Dec 3, 2020

Set up a client-side C library to talk with C APIs exposed from the compiler swiftlang/swift-driver#381

Merged

benlangmuir reviewed Dec 5, 2020

View reviewed changes

akyrtzi reviewed Dec 5, 2020

View reviewed changes

include/swift/DependencyScan/DependencyScanImpl.h Outdated Show resolved Hide resolved

akyrtzi reviewed Dec 5, 2020

View reviewed changes

include/swift/DependencyScan/DependencyScanImpl.h Outdated Show resolved Hide resolved

akyrtzi reviewed Dec 5, 2020

View reviewed changes

include/swift-c/DependencyScan/DependencyScan.h Outdated Show resolved Hide resolved

benlangmuir reviewed Dec 7, 2020

View reviewed changes

artemcm requested a review from xymus December 8, 2020 18:38

CodaFi reviewed Dec 9, 2020

View reviewed changes

artemcm mentioned this pull request Dec 10, 2020

[Explicit Module Builds] Introduce an InterModuleDependencyOracle abstraction for tracking and sharing dependency scanning results swiftlang/swift-driver#356

Merged

artemcm added 8 commits January 7, 2021 09:08

[Dependency Scanning] Bugfix: check for null C string before checking…

295e4f4

… if it is `empty`

[Dependency Scanning] Use size_t for array sizes and re-arrange arr…

58e5402

…ay struct type fields to be consistent.

[Dependency Scanning] Cache compiler instance/module depenency cache …

39483f0

…used for batched versioned-PCM scans.

[Dependency Scanning] Remove the C string getter in favour of client-…

fd03158

…side string construction (Using string length provided by the type)

Do not use Designated Initializers as they are not yet supported by a…

41f65d7

…ll of our build compilers.

[Dependency Scanning] Skip over source-code (not interface) modules w…

2121d32

…hen collecting cross import overlays

[Dependency Scanning] Update cached batch scan compiler instances wit…

94b727f

…h up-to-date search paths on successive scanner invocations To ensure they do not get stuck with stale search paths when scanning successive targets.

[Dependency Scanning] Ensure INITIALIZE_LLVM() is invoked on scanner …

cc400bf

…creation to populate LLVM's target data store.

artemcm force-pushed the DependencyScanLibrary branch from 189da80 to cc400bf Compare January 7, 2021 17:08

artemcm force-pushed the DependencyScanLibrary branch from f542987 to e991ce2 Compare January 7, 2021 22:40

artemcm force-pushed the DependencyScanLibrary branch from e991ce2 to 4f8c0e4 Compare January 7, 2021 23:45

Change file generation in the DependencyScan.ModuleDeps unittest to h…

4eae21c

…opefully work better on windows.

artemcm force-pushed the DependencyScanLibrary branch from 4f8c0e4 to 4eae21c Compare January 8, 2021 00:53

artemcm mentioned this pull request Jan 8, 2021

Rely on libSwiftScan for dependency scanning actions. swiftlang/swift-driver#427

Merged

artemcm merged commit 7e6536d into swiftlang:main Jan 8, 2021

		@@ -0,0 +1,51 @@
		swiftscan_dependency_result_get_main_module_name

Dependency Scanning Library #34786

Dependency Scanning Library #34786

Uh oh!

Conversation

artemcm commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemcm commented Nov 20, 2020

Uh oh!

artemcm commented Nov 20, 2020

Uh oh!

nkcsgexi commented Nov 20, 2020

Uh oh!

artemcm commented Nov 20, 2020

Uh oh!

artemcm commented Dec 2, 2020

Uh oh!

artemcm commented Dec 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akyrtzi commented Dec 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemcm commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemcm commented Jan 7, 2021

Uh oh!

swift-ci commented Jan 7, 2021

Uh oh!

artemcm commented Jan 7, 2021

Uh oh!

artemcm commented Jan 7, 2021

Uh oh!

artemcm commented Jan 7, 2021

Uh oh!

artemcm commented Jan 7, 2021

Uh oh!

artemcm commented Jan 8, 2021

Uh oh!

swift-ci commented Jan 8, 2021

Uh oh!

artemcm commented Jan 8, 2021

Uh oh!

artemcm commented Jan 8, 2021

Uh oh!

swift-ci commented Jan 8, 2021

Uh oh!

artemcm commented Jan 8, 2021

artemcm commented Nov 17, 2020 •

edited

Loading