[PATCH] D152356: [clang][ExtractAPI] Add --emit-symbol-graph option

[Ankur Saini via Phabricator via cfe-commits]

Arsenic created this revision.
Arsenic added a reviewer: dang.
Herald added a reviewer: ributzka.
Herald added a project: All.
Arsenic updated this revision to Diff 529841.
Arsenic added a comment.
Arsenic marked 5 inline comments as done.
Arsenic updated this revision to Diff 532145.
Arsenic updated this revision to Diff 532148.
Arsenic published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- now "--emit-symbol-garph" should emit all the symbols (even the ones external to the input files)
- update tests accordingly
- did changes as suggested in the last review


Arsenic added a comment.

- move ExtractAPIBase to it's own seperate file
- remove ExtractAPIBase::CreateExtractAPIConsumer(), consumer generation is now managed by the derived class.
- New BasicExtractAPIVisitor for including all the decls when visiting AST ( used by WrappingExtractAPIVisitor )
- New SymbolGraphConsumer for more general symbol graph generation ( emmit all the symbols )
- Refactor MacroCallBack for more general case
- New APIMacroCallBack for cases where symbol graph is generated for API information of a library ( filter out symbols from external files )


Arsenic added a comment.

remove unnecessary includes from ExtractAPIActionBase.h



================
Comment at: clang/include/clang/ExtractAPI/FrontendActions.h:21
 #include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/MultiplexConsumer.h"
 
----------------
Is this include needed here? If not it should go in the cpp file


================
Comment at: clang/include/clang/ExtractAPI/FrontendActions.h:111
+  /// craeted or not
+  bool createdASTConsumer = false;
+
----------------



================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:402
+  std::string OutputDir = CI.getFrontendOpts().SymbolGraphOutputDir;
+  if (OutputDir.empty())
+    OS = CI.createDefaultOutputFile(/*Binary=*/false, InFile,
----------------
I am not sure if the base class is the best place to put this logic in. Maybe it would make sense to have a separate implementation for the two derived classes.


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:523
+      StringRef FilePath = FIF.getFile();
+      if (auto RelativeName = getRelativeIncludeName(CI, FilePath, &IsQuoted))
+        KnownInputFiles.emplace_back(
----------------
I am not sure this does exactly what we want from this. We are hoping to get symbols that come from project headers here as well. I think that this would constrain the visitor to only record symbols that came from the input files themselves.


================
Comment at: clang/test/ExtractAPI/emit-symbol-graph/multi_file.c:8
+// RUN: %t/reference.test.json.in >> %t/reference.test.json
+// RUN: %clang %t/test.c %t/main.c -Xclang --emit-symbol-graph=%t/SymbolGraphs -Xclang --product-name=multifile_hello_world -Xclang -triple=x86_64-apple-macosx12.0.0
+
----------------
I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 

I called the driver here because it saved me an extra step of linking the standard library here.


================
Comment at: clang/test/ExtractAPI/emit-symbol-graph/multi_file.c:8
+// RUN: %t/reference.test.json.in >> %t/reference.test.json
+// RUN: %clang %t/test.c %t/main.c -Xclang --emit-symbol-graph=%t/SymbolGraphs -Xclang --product-name=multifile_hello_world -Xclang -triple=x86_64-apple-macosx12.0.0
+
----------------
Arsenic wrote:
> I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 
> 
> I called the driver here because it saved me an extra step of linking the standard library here.
> I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 
> 
> I called the driver here because it saved me an extra step of linking the standard library here.

The current tests only test if the generated symbol-graphs are correct or not, and does not check if the output file was generated or not, What would be a good way to test for the regular output file. 
Should we only test for their existence of the output files or something else also.


================
Comment at: clang/test/ExtractAPI/emit-symbol-graph/multi_file.c:8
+// RUN: %t/reference.test.json.in >> %t/reference.test.json
+// RUN: %clang %t/test.c %t/main.c -Xclang --emit-symbol-graph=%t/SymbolGraphs -Xclang --product-name=multifile_hello_world -Xclang -triple=x86_64-apple-macosx12.0.0
+
----------------
Arsenic wrote:
> Arsenic wrote:
> > I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 
> > 
> > I called the driver here because it saved me an extra step of linking the standard library here.
> > I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 
> > 
> > I called the driver here because it saved me an extra step of linking the standard library here.
> 
> The current tests only test if the generated symbol-graphs are correct or not, and does not check if the output file was generated or not, What would be a good way to test for the regular output file. 
> Should we only test for their existence of the output files or something else also.
I don't think the -Xclangs are needed here.


================
Comment at: clang/test/ExtractAPI/emit-symbol-graph/multi_file.c:8
+// RUN: %t/reference.test.json.in >> %t/reference.test.json
+// RUN: %clang %t/test.c %t/main.c -Xclang --emit-symbol-graph=%t/SymbolGraphs -Xclang --product-name=multifile_hello_world -Xclang -triple=x86_64-apple-macosx12.0.0
+
----------------
dang wrote:
> Arsenic wrote:
> > Arsenic wrote:
> > > I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 
> > > 
> > > I called the driver here because it saved me an extra step of linking the standard library here.
> > > I was a bit confused as to whether call the front end or the entire driver so currently I have created 2 different tests one for each. 
> > > 
> > > I called the driver here because it saved me an extra step of linking the standard library here.
> > 
> > The current tests only test if the generated symbol-graphs are correct or not, and does not check if the output file was generated or not, What would be a good way to test for the regular output file. 
> > Should we only test for their existence of the output files or something else also.
> I don't think the -Xclangs are needed here.
when I don't use "-Xclang" on my local machine, clang doesn't generate the required output and complaints about unused arguments.


Add new --emit-symbol-graph=<DIR> option which generates ExtractAPI symbol
graph information of .c/.m files on regular compilation job and put them in
the provided "DIR" directory.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152356

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/ExtractAPI/ExtractAPIActionBase.h
  clang/include/clang/ExtractAPI/FrontendActions.h
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  clang/test/ExtractAPI/emit-symbol-graph/multi_file.c
  clang/test/ExtractAPI/emit-symbol-graph/single_file.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D152356.532148.patch
Type: text/x-patch
Size: 41953 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230616/b02353bf/attachment-0001.bin>