This is an archive of the discontinued LLVM Phabricator instance.

[clang-scan-deps] print the dependencies to stdout and remove the need to use -MD options in the CDB
ClosedPublic

Authored by arphaman on Jun 19 2019, 4:28 PM.

Details

Summary

The gathered dependencies are now printed to the STDOUT instead of being written to disk. The tool also doesn't need the dependency output options as well to produce the dependencies.

Diff Detail

Repository
rC Clang

Event Timeline

arphaman created this revision.Jun 19 2019, 4:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 4:28 PM
aganea added inline comments.Jun 20 2019, 8:15 AM
clang/tools/clang-scan-deps/ClangScanDeps.cpp
32

Do you envision future uses for this factory?

50

Do you need the blocking behavior here? You're simply passing the Lock by reference, but not using the stream that it protects.

73

I think it would be better to keep together the lock with what it protects (thus my question above about the factory).
What about:

class SharedStream {
public:
  SharedStream(raw_ostream &OS) : OS(OS) {}
  template <typename Fn> void applyLocked(Fn f) {
    std::unique_lock<std::mutex> LockGuard(Lock);
    f(OS);
    OS.flush();
  }

private:
  std::mutex Lock;
  raw_ostream &OS;
};

/// Prints out all of the gathered dependencies into one output stream instead
/// of using the output dependency file.
class DependencyPrinter : public DependencyFileGenerator {
public:
  DependencyPrinter(std::unique_ptr<DependencyOutputOptions> Opts,
                    SharedStream &S)
      : DependencyFileGenerator(*Opts), Opts(std::move(Opts)), S(S) {}

  void finishedMainFile(DiagnosticsEngine &Diags) override {
    S.applyLocked([](raw_ostream &OS) { outputDependencyFile(OS); });
  }

private:
  std::unique_ptr<DependencyOutputOptions> Opts;
  SharedStream &S;
};

WDYT?

115

..and in that case here we would say: Compiler.addDependencyCollector(std::make_shared<DependencyPrinter>(DepOpts, SharedOS))

245

...and then here: SharedStream S(llvm::outs);.

arphaman updated this revision to Diff 205886.Jun 20 2019, 1:12 PM
arphaman marked 3 inline comments as done.

removed the lock

clang/tools/clang-scan-deps/ClangScanDeps.cpp
32

Most likely, yes.

I don't want to lock-in into creating the dependency printer in the PreprocessorOnlyTool, as the eventual goal is to have it in a library, and allow clients to consume modular dependencies not only using clang-scan-deps tool itself, but using the C++ library directly (potentially through a libclang C interface as well). The printer should be in clang-scan-deps tool itself though, so it wouldn't be ideal to reference it from PreprocessorOnlyTool now.

50

Right, I don't need it here. removed in the updated patch.

aganea added inline comments.Jun 20 2019, 1:50 PM
clang/tools/clang-scan-deps/ClangScanDeps.cpp
32

My initial question was, what are the future planned factories? (and thus the other DependencyCollector implementations?)

Do you think we could have the printer in the library as well?

On a related note - we're already testing clang-scan-deps compiled as a DLL. Our use-case right now is to call it from each of Fastbuild's worker threads. We are using main() below almost as-is (except for the multithreading part which Fastbuild already does). The CDB JSON is passed by value from the Fastbuild thread, so it doesn't go through a file. I'd like to get back dependencies-as-a-string instead of llvm::outs(). The bottleneck currently is, as you can imagine, reading / writing files (seen in ETW traces), this patch should improve things a bit. I can send a sample of how we're using it after you commit.

arphaman marked an inline comment as done.Jun 20 2019, 4:26 PM
arphaman added inline comments.
clang/tools/clang-scan-deps/ClangScanDeps.cpp
32

My initial question was, what are the future planned factories? (and thus the other DependencyCollector implementations?)

The only factory and collector right now that I have in mind is the factory that collects the dependency list from the generator, and returns them to the user of PreprocessorOnlyTool. Something like this:

class DependencyConsumer : public DependencyFileGenerator {
public:
  class Factory : public DependencyCollectorFactory {
  public:
    std::shared_ptr<DependencyCollector> createDependencyCollector(
        std::unique_ptr<DependencyOutputOptions> Opts) override {
       return DependencyConsumer(std::move(Opts));
     }
  };

  DependencyConsumer(std::unique_ptr<DependencyOutputOptions> Opts)
      : DependencyFileGenerator(*Opts), Opts(std::move(Opts)) {}

  void finishedMainFile(DiagnosticsEngine &Diags) override {
    Dependencies = getDependencies();
  }
private:
  std::vector<std::string> Dependencies;
};

But the current design is not ideal for supporting that actually. I think I'll rework it following the suggested SharedStream you mentioned earlier.

Do you think we could have the printer in the library as well?

Yes

On a related note - we're already testing clang-scan-deps compiled as a DLL. Our use-case right now is to call it from each of Fastbuild's worker threads. We are using main() below almost as-is (except for the multithreading part which Fastbuild already does).

Nice! I think the library will be a great fit for you then, the plan is to have a service object in it which will own the shared state (like the cache of the minimized sources), and worker objects which you can use in each thread to do the actual work.

arphaman updated this revision to Diff 205938.Jun 20 2019, 6:36 PM

Use the shared stream as suggested.

aganea accepted this revision.Jun 21 2019, 6:54 AM

LGTM! Thank you!

This revision is now accepted and ready to land.Jun 21 2019, 6:54 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2019, 11:21 AM