This is an archive of the discontinued LLVM Phabricator instance.

[clang][ExtractAPI] Add --emit-symbol-graph option
ClosedPublic

Authored by Arsenic on Jun 7 2023, 3:25 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Arsenic created this revision.Jun 7 2023, 3:25 AM
Herald added a project: Restricted Project. · View Herald Transcript
Arsenic added inline comments.Jun 7 2023, 3:49 AM
clang/test/ExtractAPI/emit-symbol-graph/multi_file.c
9

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.

Arsenic added inline comments.Jun 7 2023, 3:55 AM
clang/test/ExtractAPI/emit-symbol-graph/multi_file.c
9

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.

dang added inline comments.Jun 7 2023, 4:24 AM
clang/include/clang/ExtractAPI/FrontendActions.h
20

Is this include needed here? If not it should go in the cpp file

114
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
455

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.

597

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.

clang/test/ExtractAPI/emit-symbol-graph/multi_file.c
9

I don't think the -Xclangs are needed here.

Arsenic added inline comments.Jun 8 2023, 4:03 AM
clang/test/ExtractAPI/emit-symbol-graph/multi_file.c
9

when I don't use "-Xclang" on my local machine, clang doesn't generate the required output and complaints about unused arguments.

Arsenic updated this revision to Diff 529841.Jun 9 2023, 12:25 AM
  • 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 marked 5 inline comments as done.Jun 9 2023, 12:26 AM
Arsenic updated this revision to Diff 532145.Jun 16 2023, 7:39 AM
  • 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 updated this revision to Diff 532148.Jun 16 2023, 7:47 AM

remove unnecessary includes from ExtractAPIActionBase.h

Arsenic published this revision for review.Jun 16 2023, 9:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 9:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dang added inline comments.Jun 28 2023, 8:50 AM
clang/include/clang/ExtractAPI/ExtractAPIActionBase.h
26
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
227

I don't think this is needed ExtractAPIVisitor was written so that it could be used as is. If it isn't then it's a bug we should fix.

250

Not sure I like the name SymbolGraphConsumer, but I don't have a great suggestion, maybe WrappingExtractAPIConsumer so that at least it's clear that it is intended to be used by WrappingExtractAPIAction?

280

the name change here is unnecessary

562–570

I don't think we need to do all this computation since we just serialize symbol graphs for all symbols.

Arsenic updated this revision to Diff 535630.Jun 28 2023, 10:46 PM

Changes majorly include fixing issues pointed out by review comments :

  • Remove WrappingExtractAPIAction::prepareToExecuteAction()
  • Move knownInputFiles from Base action to ExtractAPIFrontendAction
  • Fix naming of MacroCallback and symbolGraphConsumer
  • Fix minor typos
  • Remove BasicExtractAPIVisitor, directly use ExtractAPIVisitor instead
Arsenic marked 5 inline comments as done.Jun 28 2023, 10:47 PM
dang accepted this revision.Jul 3 2023, 2:41 AM

LGTM thanks for working on this!

This revision is now accepted and ready to land.Jul 3 2023, 2:41 AM
This revision was landed with ongoing or failed builds.Jul 3 2023, 5:07 AM
This revision was automatically updated to reflect the committed changes.