[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#854729, @dcoughlin wrote:

> Except for some drive-by nits, this is a high-level review.
>
> I have some high-level questions about the design of the library:
>
> 1. **How do you intend to handle the case when there are multiple function definitions with the same USR?** Whose responsibility it is to deal with this: the client (for example, the static analyzer) or the index/database (libCrossTU)? This seems to me to be a fundamental part of the design for this feature that is missing. If you expect the client to handle this scenario (for example, to pick a definition) I would expect the library API to return more than a single potential result for a query. If you expect the the library to pick a definition then at a minimum you should document how this will be done. If the library will treat this as an error then you should communicate this to the client so it can attempt to recover from it.


Right now we rely on the script that is using the analyzer such as scan-build-py or codechecker to remove the duplicate USRs from the index before starting the analysis. Having an `ErrorOr` return type so the user can handle the case when multiple definitions or none of them is found could be useful. It is not feasible to return multiple definitions, however. The importing alters the ASTContext, so the only way to choose between the definitions would be via a callback that is triggered before the import is done. What do you think?

> 
> 
> 2. **Whose responsibility is it to handle errors that the library runs into?** Right now several errors appear to reported to the end user as diagnostics. It seems to me that the decision of how (or whether) to report these errors to the end user should be up to a particular client. Are these errors even actionable on the part of end users? My sense it that with the envisioned workflow users would not know/care about the format of the database file.

The intention was that to report to the user that the index format is invalid, since it is a configuration issue that she might be able to handle. But we could also return an error code and leave the reporting to the client. The reason we introduced the diagnostic is that we assumed that the client can not recover from a configuration error and will end up reporting the problem to the user.

> 
> 
> 3. **Where should the code that understands the database file format live?** Right now the logic for the parsing of the index format is in libCrossTU and the emission of the index is in the command-line tool. I think it would be easier to maintain if there were a single place that understand the file format. This way consumers and emitters of the index could be easily updated when the format/representation changes. Additionally, I think it is important to document this format.

This is a very good point! I will update the library.



================
Comment at: include/clang/Basic/DiagnosticCrossTUKinds.td:13
+def err_fnmap_parsing : Error<
+  "error parsing CrossTU index file: '%0' line: %1 'USR filename' format "
+  "expected">;
----------------
dcoughlin wrote:
> Is this a user-facing diagnostic? Do we expect users to know what 'CrossTU' means? Or what 'USR' means?
It is. Do you have any alternative suggestion? What about `error parsing index file` without mentioning any of the terms you mentioned?


================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:60
+  ///
+  /// Note that the AST files should also be in the \p CrossTUDir.
+  const FunctionDecl *getCrossTUDefinition(const FunctionDecl *FD,
----------------
dcoughlin wrote:
> Can this document what happens when the function declaration cannot be found? And what happens when multiple function declarations are found?
Sure. Good point.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:48
+                                                       StringRef LookupFnName) {
+  if (!DC)
+    return nullptr;
----------------
dcoughlin wrote:
> Should this assert that DC is not null? What is the reason to accept null here?
Indeed, an assert should be better. 


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+      if (!ExternalFnMapFile) {
+        Context.getDiagnostics().Report(diag::err_fe_error_opening)
+            << ExternalFunctionMap << "required by the CrossTU functionality";
----------------
dcoughlin wrote:
> What is the rationale behind emitting this as a diagnostic? In general for a library I would expect that errors would be communicated to the client, which then would have responsibility for handling them, reporting them to the user, or aborting.
Since this is likely to be a configuration error we were thinking that only the user can handle it. But I could transform this to an error code if you would prefer that solution. 


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:92
+        StringRef LineRef{Line};
+        if (Pos > 0 && Pos != std::string::npos) {
+          FunctionName = LineRef.substr(0, Pos);
----------------
dcoughlin wrote:
> It looks like this is parsing. Is the file format documented anywhere? Also, it would be good to keep the parsing and generation code  in the same place so that it is easy to figure out what to update when the file format changes.
Good point!


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:86
+          if (SM.isInMainFile(Body->getLocStart()))
+            DefinedFuncsStr << LookupName.str().str() << " " << CurrentFileName
+                            << "\n";
----------------
dcoughlin wrote:
> It seems like the functionality that writes an entry and the functionality that reads an entry should ideally be in the same place. This way when the format is updated it is obvious what other places need to be updated. Can the code that writes a mapping be moved to libCrossTU and can we have this tool link against it?
Sure, good point!


https://reviews.llvm.org/D34512