Improved import of 'C implicit' functions. #689

balazske · 2019-07-09T08:17:28Z

No description provided.

balazske · 2019-07-09T08:20:08Z

This fix still does not fix the error for #684 but maybe it is useful.

martong

The naming implicit function is really ambiguous. Clang AST contains several nodes (including functions) with the implicit tag. E.g. auto generated constructors, dtors are referred and signed as "implicit" in the AST. We even have an accessor function for that in the AST, see Decl::isImplicit

  /// isImplicit - Indicates whether the declaration was implicitly
  /// generated by the implementation. If false, this declaration
  /// was written explicitly in the source code.
  bool isImplicit() const { return Implicit; }

I think we should come up with a new naming for this both in the implementation and in the tests. We may call them something like "C function without prototype" (CFunctionWithoutPrototype) ?

Also, calling functions without prototype is not allowed in C99 and in C11, so this seems like a very borderline feature, I am not sure if we should spend so much time with this.

martong · 2019-07-09T09:18:32Z

lib/AST/ASTImporter.cpp

@@ -3048,9 +3047,12 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
  }
  // Try to find a function in our own ("to") context with the same name, same
  // type, and in the same context as the function we're importing.
-  else if (!LexicalDC->isFunctionOrMethod()) {
+  else {
+    bool IsCImplicit = LexicalDC->isFunctionOrMethod() && D->isImplicit();


We could have a lambda for this, which we could reuse at line 3070: if (IsCImplicit(D) && IsCImplicit(FoundFunction)) { ... }
Note, probably we want to check the Lexical decl in the "from" context, and not in the "to" context, LexicalDC is in the "to" context while D is in the "from" ctx.

Also, it would be nice to explain what do we mean under "C implicit" functions, perhaps with an example or with a reference to the paragraph in the C standard.

martong · 2019-07-09T09:23:16Z

lib/AST/ASTImporterLookupTable.cpp

+  bool VisitCallExpr(CallExpr *E) {
+    FunctionDecl *Callee = E->getDirectCallee();
+    // Our case: Declaration that is inside a function and implicit.
+    if (Callee && Callee->getLexicalDeclContext()->isFunctionOrMethod() &&


Add a function isCImplicit() ?

Probably not needed to add it to FunctionDecl because it is a rare case. And I am not sure if the conditions (for getLexicalDeclContext and isImplicit) are always correct and only true for this case.

martong · 2019-07-09T09:30:44Z

unittests/AST/ASTImporterTest.cpp

+  auto *ToF2 = Import(FromF2, Lang_C);
+  ASSERT_TRUE(ToImplicitF1);
+  EXPECT_EQ(GetImplicitF(ToF1), ToImplicitF1);
+  EXPECT_EQ(GetImplicitF(ToF2), ToImplicitF1);


Should there be a redecl chain here?
I'd expect that when we import f2 then we find the existing implicit_f so the newly imported implict_f should be linked to that as an additional decl in the same redecl chain.

Yes I would expect it too but the AST dump shows that there is no chain. In this code the same FunctionDecl is used in f1 and f2 (source location returned by getLocation is valid and points to the first occurrence in f1).
I do not know if it is problem is we create the redecl chain at import even if originally there is no chain. This would mean create a new FunctionDecl instead of the MapImported in ASTImporter. (Only to have less special cases in the code that may cause other new problems.)

If the "from" AST does not have a redecl chain, then probably we should not have either, but then we should have an assert_xx for the no chain in the "from" ctx.

martong · 2019-07-09T09:32:19Z

unittests/AST/ASTImporterTest.cpp

+
+  auto *ToImplicitF2 = Import(FromImplicitF2, Lang_C);
+  ASSERT_TRUE(ToImplicitF2);
+  EXPECT_EQ(ToImplicitF2, ToImplicitF1);


I'd expect a redecl chain here as well.

martong · 2019-07-09T09:45:31Z

This fix still does not fix the error for #684 but maybe it is useful.

Calling functions without prototype is not allowed in C99 and in C11 (AFAIK), so this seems like a very borderline feature, I am not sure if we should spend so much time with this.

balazske · 2019-07-09T10:40:42Z

We can wait until the source of problems in #684 is found, probably this PR is not needed then (but still some old code can use the "C implicit function" feature) (but it could be a bug in the code too: the function is missing from the include file).

8000

balazske · 2019-07-09T10:41:56Z

Or we can generate the "UnsupportedConstruct" error for this case.

martong · 2019-07-09T12:55:59Z

We can wait until the source of problems in #684 is found, probably this PR is not needed then (but still some old code can use the "C implicit function" feature) (but it could be a bug in the code too: the function is missing from the include file).

Let's see if we can find the real root cause of #684. If there is no actual crash related to "C functions without prototype" then we might not need these changes.

balazske · 2019-07-09T14:45:27Z

The test "ctu-main.c" contains this:

// The external function prototype is incomplete.
// warning:implicit functions are prohibited by c99
void testImplicit() {
  int res = identImplicit(6);   // external implicit functions are not inlined
  clang_analyzer_eval(res == 6); // expected-warning{{TRUE}}

The comment says "external implicit functions are not inlined" but this is not true (without the fix too) because the function is inlined if it is found in the externapDefMap file. This test should be changed, or really the external implicit function should not be inlined?
The root cause of the SourceLocation problem in #684 is really that there is not a valid SourceLocation if the function is external implicit (but it is inlined because it is visited by the visitor). So that fix is correct if we want these functions to be inlined.

martong · 2019-07-09T15:15:08Z

The test "ctu-main.c" contains this:
// The external function prototype is incomplete.
// warning:implicit functions are prohibited by c99
void testImplicit() {
  int res = identImplicit(6);   // external implicit functions are not inlined
  clang_analyzer_eval(res == 6); // expected-warning{{TRUE}}
The comment says "external implicit functions are not inlined" but this is not true (without the fix too) because the function is inlined if it is found in the externapDefMap file. This test should be changed, or really the external implicit function should not be inlined?

I think we should inline it, could you please change the comment in the test?

The root cause of the SourceLocation problem in #684 is really that there is not a valid SourceLocation if the function is external implicit (but it is inlined because it is visited by the visitor). So that fix is correct if we want these functions to be inlined.

So, if I understand it correctly then #684 will be solved by correctly importing the "external implicit" C functions without prototypes?
If that is the case then let's continue with this PR. Though I am confused now, because previously you wrote that this PR would not solve #684.

balazske · 2019-07-09T15:37:41Z

The fix in this PR makes the links between multiple FunctionDecls (for same function) correct. Probably the analyzer still works correct by using the inlined definition for analysis but the "CallEvent" contains the CallExpr that contains the original FunctionDecl (regardless if there is a link to the definition of the inlined function). The same conditions appear if the fix in this PR is applied or not, the difference is that the getDefinition returns the correct inlined definition or not. In this way, this fix does not make something work that was wrong before (but makes the imported AST more correct, maybe later some problem appears that needs this fix).

martong · 2019-07-09T15:48:03Z

Ok.

Now I don't understand why we hadn't have any assertion before when we run the ctu-main.c test ? We do inline the "external implicit" identImplicit function there and without a SourceLocation, right?

martong · 2019-07-09T15:54:27Z

Perhaps that is because there is no bug reported, so the BugReporterVisitor is not invoked.

Anyway this observation makes it cleaner to synthesize a test for #684:
We could have something similar what we have in ctu-main.c with identImplicit but in the other TU where identImplicit is defined we could make a division by zero. Then the bug path would go through the callExpr of idendImplicit ...

balazske · 2019-07-10T09:21:32Z

Finally in #690 there is a test to reproduce the problem in #684.
This fix is independent of those problems because these happens even with this fix applied. Still it is an improvement to ASTImporter so it can be useful.

Improved import of 'C implicit' functions.

53a93fd

martong suggested changes Jul 9, 2019

View reviewed changes

Added comments and renames.

8566783

martong approved these changes Jul 10, 2019

View reviewed changes

balazske merged commit 446c84d into Ericsson:ctu-clang8 Jul 10, 2019

martong pushed a commit that referenced this pull request Sep 17, 2019

Improved import of 'C implicit' functions. (#689)

3c6cb18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improved import of 'C implicit' functions. #689

Improved import of 'C implicit' functions. #689

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Improved import of 'C implicit' functions. #689

Improved import of 'C implicit' functions. #689

Uh oh!

Conversation

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!